fix(core): target defaults application shouldn't include extra scripts (

#21970)
nrwl · Feb 23, 2024 · 81a347d · 81a347d
1 parent e0077ca
commit 81a347d
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 92 deletions.
diff --git a/packages/nx/src/plugins/target-defaults/target-defaults-plugin.spec.ts b/packages/nx/src/plugins/target-defaults/target-defaults-plugin.spec.ts
@@ -200,7 +200,7 @@ describe('target-defaults plugin', () => {
     ).toMatchInlineSnapshot(`{}`);
   });
 
-  it('should not add target if project does not define target', () => {
+  it('should only modify target if package json has script but its not included', () => {
     memfs.vol.fromJSON(
       {
         'package.json': JSON.stringify({
@@ -234,10 +234,6 @@ describe('target-defaults plugin', () => {
             "targets": {
               "test": {
                 "command": "jest",
-                "executor": "nx:run-script",
-                "options": {
-                  "script": "test",
-                },
                 Symbol(ONLY_MODIFIES_EXISTING_TARGET): true,
               },
             },
@@ -381,10 +377,8 @@ describe('target-defaults plugin', () => {
       const result = getTargetInfo(
         'echo',
         {
-          targets: {
-            echo: {
-              command: 'echo hi',
-            },
+          echo: {
+            command: 'echo hi',
           },
         },
         null
@@ -400,13 +394,11 @@ describe('target-defaults plugin', () => {
       const result = getTargetInfo(
         'echo',
         {
-          targets: {
-            echo: {
-              executor: 'nx:run-commands',
-              options: {
-                command: 'echo hi',
-                cwd: '{projectRoot}',
-              },
+          echo: {
+            executor: 'nx:run-commands',
+            options: {
+              command: 'echo hi',
+              cwd: '{projectRoot}',
             },
           },
         },
@@ -425,32 +417,10 @@ describe('target-defaults plugin', () => {
     it('should include script for run-script', () => {
       expect(
         getTargetInfo('build', null, {
-          scripts: {
-            build: 'echo hi',
-          },
-        })
-      ).toMatchInlineSnapshot(`
-        {
-          "executor": "nx:run-script",
-          "options": {
-            "script": "build",
-          },
-        }
-      `);
-
-      expect(
-        getTargetInfo('echo', null, {
-          scripts: {
-            build: 'echo hi',
-          },
-          nx: {
-            targets: {
-              echo: {
-                executor: 'nx:run-script',
-                options: {
-                  script: 'build',
-                },
-              },
+          build: {
+            executor: 'nx:run-script',
+            options: {
+              script: 'build',
             },
           },
         })

diff --git a/packages/nx/src/plugins/target-defaults/target-defaults-plugin.ts b/packages/nx/src/plugins/target-defaults/target-defaults-plugin.ts
@@ -61,16 +61,15 @@ export const TargetDefaultsPlugin: NxPluginV2 = {
       const packageJson = readJsonOrNull<PackageJson>(
         join(ctx.workspaceRoot, root, 'package.json')
       );
+      const packageJsonTargets = readTargetsFromPackageJson(packageJson);
       const projectDefinedTargets = new Set([
         ...Object.keys(projectJson?.targets ?? {}),
-        ...(packageJson
-          ? Object.keys(readTargetsFromPackageJson(packageJson))
-          : []),
+        ...(packageJson ? Object.keys(packageJsonTargets) : []),
       ]);
 
       const executorToTargetMap = getExecutorToTargetMap(
-        packageJson,
-        projectJson
+        packageJsonTargets,
+        projectJson?.targets
       );
 
       const modifiedTargets: Record<
@@ -90,7 +89,11 @@ export const TargetDefaultsPlugin: NxPluginV2 = {
               JSON.stringify(targetDefaults[defaultSpecifier])
             );
             modifiedTargets[targetName] = {
-              ...getTargetInfo(targetName, projectJson, packageJson),
+              ...getTargetInfo(
+                targetName,
+                projectJson?.targets,
+                packageJsonTargets
+              ),
               ...defaults,
             };
           }
@@ -114,20 +117,20 @@ export const TargetDefaultsPlugin: NxPluginV2 = {
 };
 
 function getExecutorToTargetMap(
-  packageJson: PackageJson,
-  projectJson: ProjectConfiguration
+  packageJsonTargets: Record<string, TargetConfiguration>,
+  projectJsonTargets: Record<string, TargetConfiguration>
 ) {
   const executorToTargetMap = new Map<string, Set<string>>();
   const targets = Object.keys({
-    ...projectJson?.targets,
-    ...packageJson?.scripts,
-    ...packageJson?.nx?.targets,
+    ...projectJsonTargets,
+    ...packageJsonTargets,
   });
   for (const target of targets) {
-    const executor =
-      projectJson?.targets?.[target]?.executor ??
-      packageJson?.nx?.targets?.[target]?.executor ??
-      'nx:run-script';
+    const executor = getTargetExecutor(
+      target,
+      projectJsonTargets,
+      packageJsonTargets
+    );
     const targetsForExecutor = executorToTargetMap.get(executor) ?? new Set();
     targetsForExecutor.add(target);
     executorToTargetMap.set(executor, targetsForExecutor);
@@ -154,13 +157,17 @@ function readJsonOrNull<T extends Object = any>(path: string) {
  */
 export function getTargetInfo(
   target: string,
-  projectJson: Pick<ProjectConfiguration, 'targets'>,
-  packageJson: Pick<PackageJson, 'scripts' | 'nx'>
+  projectJsonTargets: Record<string, TargetConfiguration>,
+  packageJsonTargets: Record<string, TargetConfiguration>
 ) {
-  const projectJsonTarget = projectJson?.targets?.[target];
-  const packageJsonTarget = packageJson?.nx?.targets?.[target];
-
-  const executor = getTargetExecutor(target, projectJson, packageJson);
+  const projectJsonTarget = projectJsonTargets?.[target];
+  const packageJsonTarget = packageJsonTargets?.[target];
+
+  const executor = getTargetExecutor(
+    target,
+    projectJsonTargets,
+    packageJsonTargets
+  );
   const targetOptions = {
     ...packageJsonTarget?.options,
     ...projectJsonTarget?.options,
@@ -211,24 +218,23 @@ export function getTargetInfo(
 
 function getTargetExecutor(
   target: string,
-  projectJson: Pick<ProjectConfiguration, 'targets'>,
-  packageJson: Pick<PackageJson, 'scripts' | 'nx'>
+  projectJsonTargets: Record<string, TargetConfiguration>,
+  packageJsonTargets: Record<string, TargetConfiguration>
 ) {
-  const projectJsonTarget = projectJson?.targets?.[target];
-  const packageJsonTarget = packageJson?.nx?.targets?.[target];
-  const packageJsonScript = packageJson?.scripts?.[target];
+  const projectJsonTargetConfiguration = projectJsonTargets?.[target];
+  const packageJsonTargetConfiguration = packageJsonTargets?.[target];
 
-  if (projectJsonTarget?.command) {
-    return 'nx:run-commands';
+  if (!projectJsonTargetConfiguration && packageJsonTargetConfiguration) {
+    return packageJsonTargetConfiguration?.executor ?? 'nx:run-script';
   }
 
-  if (
-    !projectJsonTarget?.executor &&
-    !packageJsonTarget?.executor &&
-    packageJsonScript
-  ) {
-    return 'nx:run-script';
+  if (projectJsonTargetConfiguration?.executor) {
+    return projectJsonTargetConfiguration.executor;
+  }
+
+  if (projectJsonTargetConfiguration?.command) {
+    return 'nx:run-commands';
   }
 
-  return projectJsonTarget?.executor ?? packageJsonTarget?.executor;
+  return null;
 }
diff --git a/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts b/packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts
@@ -69,6 +69,32 @@ describe('project-configuration-utils', () => {
       ).toEqual({ executor: 'target2', outputs: ['output1'] });
     });
 
+    it('should not overwrite target with information from defaults', () => {
+      const result = mergeTargetConfigurations(
+        {
+          executor: 'foo',
+          options: {
+            baz: true,
+          },
+          [ONLY_MODIFIES_EXISTING_TARGET]: true,
+        } as any,
+        {
+          executor: 'bar',
+          options: {
+            bang: true,
+          },
+        }
+      );
+      expect(result).toMatchInlineSnapshot(`
+        {
+          "executor": "bar",
+          "options": {
+            "bang": true,
+          },
+        }
+      `);
+    });
+
     describe('options', () => {
       it('should merge if executor matches', () => {
         expect(

diff --git a/packages/nx/src/project-graph/utils/project-configuration-utils.ts b/packages/nx/src/project-graph/utils/project-configuration-utils.ts
@@ -155,20 +155,21 @@ export function mergeProjectConfigurationIntoRootMap(
         continue;
       }
 
+      const mergedTarget = mergeTargetConfigurations(
+        skipCommandNormalization
+          ? target
+          : resolveCommandSyntacticSugar(target, project.root),
+        matchingProject.targets?.[targetName],
+        sourceMap,
+        sourceInformation,
+        `targets.${targetName}`
+      );
+
       // We don't want the symbol to live on past the merge process
-      if (target?.[ONLY_MODIFIES_EXISTING_TARGET])
-        delete target?.[ONLY_MODIFIES_EXISTING_TARGET];
-
-      updatedProjectConfiguration.targets[targetName] =
-        mergeTargetConfigurations(
-          skipCommandNormalization
-            ? target
-            : resolveCommandSyntacticSugar(target, project.root),
-          matchingProject.targets?.[targetName],
-          sourceMap,
-          sourceInformation,
-          `targets.${targetName}`
-        );
+      if (mergedTarget?.[ONLY_MODIFIES_EXISTING_TARGET])
+        delete mergedTarget?.[ONLY_MODIFIES_EXISTING_TARGET];
+
+      updatedProjectConfiguration.targets[targetName] = mergedTarget;
     }
   }
 
@@ -434,6 +435,14 @@ export function mergeTargetConfigurations(
   // in both places. This means that it is likely safe to merge
   const isCompatible = isCompatibleTarget(baseTargetProperties, target);
 
+  // If the targets are not compatible, we would normally overwrite the old target
+  // with the new one. However, we have a special case for targets that have the
+  // ONLY_MODIFIES_EXISTING_TARGET symbol set. This prevents the merged target
+  // equaling info that should have only been used to modify the existing target.
+  if (!isCompatible && target[ONLY_MODIFIES_EXISTING_TARGET]) {
+    return baseTarget;
+  }
+
   if (!isCompatible && projectConfigSourceMap) {
     // if the target is not compatible, we will simply override the options
     // we have to delete old entries from the source map

diff --git a/packages/nx/src/utils/package-json.ts b/packages/nx/src/utils/package-json.ts
@@ -124,7 +124,7 @@ export function buildTargetFromScript(script: string): TargetConfiguration {
 }
 
 export function readTargetsFromPackageJson(packageJson: PackageJson) {
-  const { scripts, nx } = packageJson;
+  const { scripts, nx, private: isPrivate } = packageJson ?? {};
   const res: Record<string, TargetConfiguration> = {};
   const includedScripts = nx?.includedScripts || Object.keys(scripts ?? {});
   //
@@ -143,7 +143,7 @@ export function readTargetsFromPackageJson(packageJson: PackageJson) {
    * not marked as `"private": true` to allow for lightweight configuration for
    * package based repos.
    */
-  if (!packageJson.private && !res['nx-release-publish']) {
+  if (!isPrivate && !res['nx-release-publish']) {
     res['nx-release-publish'] = {
       dependsOn: ['^nx-release-publish'],
       executor: '@nx/js:release-publish',