From 7d844c4077256165c8d20107a8ee3a8642653b4c Mon Sep 17 00:00:00 2001 From: AgentEnder Date: Thu, 29 Dec 2022 16:19:35 -0500 Subject: [PATCH] chore(misc): address review feedback --- docs/shared/reference/nx-json.md | 20 +- packages/nx/src/config/workspaces.spec.ts | 248 ++++++++++++++++++---- packages/nx/src/config/workspaces.ts | 90 +++++--- 3 files changed, 276 insertions(+), 82 deletions(-) diff --git a/docs/shared/reference/nx-json.md b/docs/shared/reference/nx-json.md index badb07edac2a7e..f0d4f86db1d73a 100644 --- a/docs/shared/reference/nx-json.md +++ b/docs/shared/reference/nx-json.md @@ -115,7 +115,6 @@ In this case Nx will use the right `production` input for each project. Target defaults provide ways to set common options for a particular target in your workspace. When building your project's configuration, we merge it with up to 1 default from this map. For a given target, we look at its name and its executor. We then check target defaults for any of the following combinations: -- `` `${targetName}#${executor}` `` - `` `${executor}` `` - `` `${targetName}` `` @@ -157,6 +156,25 @@ Another target default you can configure is `outputs`: } ``` +When defining any options or configurations inside of a target default, you may use the `{workspaceRoot}` and `{projectRoot}` tokens. This is useful for defining things like the outputPath or tsconfig for many build targets: + +```json {% fileName="nx.json" %} +{ + "targetDefaults": { + "@nrwl/js:tsc": { + "options": { + "main": "{projectRoot}/src/index.ts" + }, + "configurations": { + "prod": { + "tsconfig": "{projectRoot}/tsconfig.prod.json" + } + } + } + } +} +``` + ### Generators Default generator options are configured in `nx.json` as well. For instance, the following tells Nx to always diff --git a/packages/nx/src/config/workspaces.spec.ts b/packages/nx/src/config/workspaces.spec.ts index f02d528357cc17..8360f7e1ca90cf 100644 --- a/packages/nx/src/config/workspaces.spec.ts +++ b/packages/nx/src/config/workspaces.spec.ts @@ -8,6 +8,7 @@ import { NxJsonConfiguration } from './nx-json'; import { vol } from 'memfs'; import * as fastGlob from 'fast-glob'; +import { TargetConfiguration } from './workspace-json-project-json'; jest.mock('fs', () => require('memfs').fs); @@ -146,15 +147,15 @@ describe('Workspaces', () => { describe('target defaults', () => { const targetDefaults = { - 'build#nx:run-commands': { - options: { - key: 't:e', - }, - }, 'nx:run-commands': { options: { key: '*:e', }, + configurations: { + ['test-config']: { + val: 'b', + }, + }, }, build: { options: { @@ -163,13 +164,6 @@ describe('Workspaces', () => { }, }; - it('should prefer target#executor key', () => { - expect( - readTargetDefaultsForTarget('build', targetDefaults, 'nx:run-commands') - .options['key'] - ).toEqual('t:e'); - }); - it('should prefer executor key', () => { expect( readTargetDefaultsForTarget( @@ -197,9 +191,8 @@ describe('Workspaces', () => { ).toBeNull(); }); - it.each(['configurations', 'options'])( - 'should merge %s if executor matches', - (property) => { + describe('options', () => { + it('should merge if executor matches', () => { expect( mergeTargetConfigurations( { @@ -207,7 +200,7 @@ describe('Workspaces', () => { targets: { build: { executor: 'target', - [property]: { + options: { a: {}, }, }, @@ -216,27 +209,24 @@ describe('Workspaces', () => { 'build', { executor: 'target', - [property]: { + options: { a: 'overriden', b: {}, }, } - )[property] + ).options ).toEqual({ a: {}, b: {} }); - } - ); + }); - it.each(['configurations', 'options'])( - 'should merge %s if executor is only provided by target', - (property) => { + it('should merge if executor is only provided on the project', () => { expect( mergeTargetConfigurations( { - root: '', + root: '.', targets: { build: { executor: 'target', - [property]: { + options: { a: {}, }, }, @@ -244,25 +234,23 @@ describe('Workspaces', () => { }, 'build', { - [property]: { + options: { + a: 'overriden', b: {}, }, } - )[property] + ).options ).toEqual({ a: {}, b: {} }); - } - ); + }); - it.each(['configurations', 'options'])( - 'should merge %s if executor is only provided by defaults', - (property) => { + it('should merge if executor is only provided in the defaults', () => { expect( mergeTargetConfigurations( { - root: '', + root: '.', targets: { build: { - [property]: { + options: { a: {}, }, }, @@ -271,25 +259,24 @@ describe('Workspaces', () => { 'build', { executor: 'target', - [property]: { + options: { + a: 'overriden', b: {}, }, } - )[property] + ).options ).toEqual({ a: {}, b: {} }); - } - ); + }); - it.each(['configurations', 'options'])( - 'should not merge %s if executor is only provided by defaults', - (property) => { + it('should not merge if executor is only provided by defaults', () => { expect( mergeTargetConfigurations( { root: '', targets: { build: { - [property]: { + executor: 'other', + options: { a: {}, }, }, @@ -298,13 +285,180 @@ describe('Workspaces', () => { 'build', { executor: 'target', - [property]: { + options: { b: {}, }, } - )[property] - ).toEqual({ a: {}, b: {} }); - } - ); + ).options + ).toEqual({ a: {} }); + }); + + it('should resolve workspaceRoot and projectRoot tokens', () => { + expect( + mergeTargetConfigurations( + { + root: 'my/project', + targets: { + build: { + options: { + a: '{workspaceRoot}', + }, + }, + }, + }, + 'build', + { + executor: 'target', + options: { + b: '{workspaceRoot}/dist/{projectRoot}', + }, + } + ).options + ).toEqual({ a: '{workspaceRoot}', b: 'dist/my/project' }); + }); + }); + + describe('configurations', () => { + const projectConfigurations: TargetConfiguration['configurations'] = { + dev: { + foo: '1', + }, + prod: { + bar: '2', + }, + }; + + const defaultConfigurations: TargetConfiguration['configurations'] = { + dev: { + foo: 'z', + other: '2', + }, + baz: { + x: '3', + }, + }; + + const merged: TargetConfiguration['configurations'] = { + dev: { + foo: '1', + other: '2', + }, + prod: { bar: '2' }, + baz: { x: '3' }, + }; + + it('should merge configurations if executor matches', () => { + expect( + mergeTargetConfigurations( + { + root: '.', + targets: { + build: { + executor: 'target', + configurations: projectConfigurations, + }, + }, + }, + 'build', + { + executor: 'target', + configurations: defaultConfigurations, + } + ).configurations + ).toEqual(merged); + }); + + it('should merge if executor is only provided on the project', () => { + expect( + mergeTargetConfigurations( + { + root: '.', + targets: { + build: { + executor: 'target', + configurations: projectConfigurations, + }, + }, + }, + 'build', + { + configurations: defaultConfigurations, + } + ).configurations + ).toEqual(merged); + }); + + it('should merge if executor is only provided in the defaults', () => { + expect( + mergeTargetConfigurations( + { + root: '.', + targets: { + build: { + configurations: projectConfigurations, + }, + }, + }, + 'build', + { + executor: 'target', + configurations: defaultConfigurations, + } + ).configurations + ).toEqual(merged); + }); + + it('should not merge if executor doesnt match', () => { + expect( + mergeTargetConfigurations( + { + root: '', + targets: { + build: { + executor: 'other', + configurations: projectConfigurations, + }, + }, + }, + 'build', + { + executor: 'target', + configurations: defaultConfigurations, + } + ).configurations + ).toEqual(projectConfigurations); + }); + + it('should resolve workspaceRoot and projectRoot tokens', () => { + expect( + mergeTargetConfigurations( + { + root: 'my/project', + targets: { + build: { + configurations: { + dev: { + a: '{workspaceRoot}', + }, + }, + }, + }, + }, + 'build', + { + executor: 'target', + configurations: { + prod: { + a: '{workspaceRoot}/dist/{projectRoot}', + }, + }, + } + ).configurations + ).toEqual({ + dev: { a: '{workspaceRoot}' }, + prod: { a: 'dist/my/project' }, + }); + }); + }); }); }); diff --git a/packages/nx/src/config/workspaces.ts b/packages/nx/src/config/workspaces.ts index 04237fce9b69ab..97ce2680389327 100644 --- a/packages/nx/src/config/workspaces.ts +++ b/packages/nx/src/config/workspaces.ts @@ -7,7 +7,7 @@ import { performance } from 'perf_hooks'; import { workspaceRoot } from '../utils/workspace-root'; import { readJsonFile } from '../utils/fileutils'; -import { logger, NX_PREFIX } from '../utils/logger'; +import { logger, NX_PREFIX, stripIndent } from '../utils/logger'; import { loadNxPlugins, readPluginPackageJson } from '../utils/nx-plugin'; import * as yaml from 'js-yaml'; @@ -938,7 +938,14 @@ export function mergeTargetConfigurations( targetDefaults: TargetDefaults[string] ): TargetConfiguration { const { targets, root } = projectConfiguration; - const targetConfiguration = targets?.[target] ?? {}; + const targetConfiguration = targets?.[target]; + + if (!targetConfiguration) { + throw new Error( + `Attempted to merge targetDefaults for ${projectConfiguration.name}.${target}, which doesn't exist.` + ); + } + const { configurations: defaultConfigurations, options: defaultOptions, @@ -949,34 +956,59 @@ export function mergeTargetConfigurations( ...targetConfiguration, }; - if ( - !targetDefaults.executor && - !targetConfiguration.executor && - !targetDefaults.command && - !targetConfiguration.command - ) { - throw new Error('Unable to determine executor for target'); - } - - // Target is "homogenous", e.g. executor is defined only once or is the same - // in both places + // Target is "compatible", e.g. executor is defined only once or is the same + // in both places. This means that it is likely safe to merge options if ( !targetDefaults.executor || !targetConfiguration.executor || targetDefaults.executor === targetConfiguration.executor ) { - result.options = { - ...resolvePathTokensInOptions(defaultOptions, root, target), - ...targetConfiguration.options, - }; - result.configurations = { - ...defaultConfigurations, - ...targetConfiguration.configurations, - }; + result.options = mergeOptions( + defaultOptions, + targetConfiguration.options ?? {}, + root, + target + ); + result.configurations = mergeConfigurations( + defaultConfigurations, + targetConfiguration.configurations, + root, + target + ); } return result as TargetConfiguration; } +function mergeOptions( + defaults: T, + options: T, + projectRoot: string, + key: string +): T { + return { + ...resolvePathTokensInOptions(defaults, projectRoot, key), + ...options, + }; +} + +function mergeConfigurations( + defaultConfigurations: Record, + projectDefinedConfigurations: Record, + projectRoot: string, + targetName: string +): Record { + const configurations: Record = { ...projectDefinedConfigurations }; + for (const configuration in defaultConfigurations) { + configurations[configuration] = mergeOptions( + defaultConfigurations[configuration], + configurations[configuration], + projectRoot, + `${targetName}.${configuration}` + ); + } + return configurations; +} + function resolvePathTokensInOptions( object: T, projectRoot: string, @@ -988,12 +1020,8 @@ function resolvePathTokensInOptions( value = value.replace(/^\{workspaceRoot\}\//, ''); } if (value.includes('{workspaceRoot}')) { - output.error({ - title: `${NX_PREFIX}: {workspaceRoot} is only valid at the beginning of an option`, - bodyLines: [`Found {workspaceRoot} in property: ${key}`], - }); throw new Error( - '{workspaceRoot} is only valid at the beginning of an option.' + `${NX_PREFIX} The {workspaceRoot} token is only valid at the beginning of an option. (${key})` ); } object[opt] = value.replace('{projectRoot}', projectRoot); @@ -1013,15 +1041,9 @@ export function readTargetDefaultsForTarget( // If an executor is defined in project.json, defaults should be read // from the most specific key that matches that executor. // e.g. If executor === run-commands, and the target is named build: - // Use build#nx:run-commands if it is present - // If not, use nx:run-commands if it is present + // Use, use nx:run-commands if it is present // If not, use build if it is present. - const separator = '#'; - const key = [ - `${targetName}${separator}${executor}`, - executor, - targetName, - ].find((x) => targetDefaults?.[x]); + const key = [executor, targetName].find((x) => targetDefaults?.[x]); return key ? targetDefaults?.[key] : null; } else { // If the executor is not defined, the only key we have is the target name.