Skip to content

Commit

Permalink
fix(core): address some wonkiness when merging command and run-commands
Browse files Browse the repository at this point in the history
  • Loading branch information
@AgentEnder
AgentEnder committed Jan 24, 2024
1 parent 70fd808 commit 1ab03ad
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 102 deletions.
31 changes: 0 additions & 31 deletions packages/nx/src/project-graph/utils/normalize-project-nodes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,6 @@ export function normalizeProjectTargets(
continue;
}

targets[target] = resolveCommandSyntacticSugar(
targets[target],
`${projectName}:${target}`
);

targets[target].options = resolveNxTokensInOptions(
targets[target].options,
project,
Expand Down Expand Up @@ -134,29 +129,3 @@ export function normalizeImplicitDependencies(
.concat(implicitDependencies.filter((x) => x.startsWith('!')))
);
}

function resolveCommandSyntacticSugar(
target: TargetConfiguration,
key: string
): TargetConfiguration {
const { command, ...config } = target ?? {};

if (!command) {
return target;
}

if (config.executor) {
throw new Error(
`${NX_PREFIX} ${key} should not have executor and command both configured.`
);
} else {
return {
...config,
executor: 'nx:run-commands',
options: {
...config.options,
command: command,
},
};
}
}
145 changes: 108 additions & 37 deletions packages/nx/src/project-graph/utils/project-configuration-utils.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,106 @@ describe('project-configuration-utils', () => {
).toEqual(projectDefaultConfiguration);
});
});

describe('run-commands', () => {
it('should merge two run-commands targets appropriately', () => {
const merged = mergeTargetConfigurations(
{
outputs: ['{projectRoot}/outputfile.json'],
options: {
command: 'eslint . -o outputfile.json',
},
},
{
cache: true,
inputs: [
'default',
'{workspaceRoot}/.eslintrc.json',
'{workspaceRoot}/apps/third-app/.eslintrc.json',
'{workspaceRoot}/tools/eslint-rules/**/*',
{ externalDependencies: ['eslint'] },
],
options: { cwd: 'apps/third-app', command: 'eslint .' },
executor: 'nx:run-commands',
configurations: {},
}
);
expect(merged).toMatchInlineSnapshot(`
{
"cache": true,
"configurations": {},
"executor": "nx:run-commands",
"inputs": [
"default",
"{workspaceRoot}/.eslintrc.json",
"{workspaceRoot}/apps/third-app/.eslintrc.json",
"{workspaceRoot}/tools/eslint-rules/**/*",
{
"externalDependencies": [
"eslint",
],
},
],
"options": {
"command": "eslint . -o outputfile.json",
"cwd": "apps/third-app",
},
"outputs": [
"{projectRoot}/outputfile.json",
],
}
`);
});

it('should merge targets when the base uses command syntactic sugar', () => {
const merged = mergeTargetConfigurations(
{
outputs: ['{projectRoot}/outputfile.json'],
options: {
command: 'eslint . -o outputfile.json',
},
},
{
cache: true,
inputs: [
'default',
'{workspaceRoot}/.eslintrc.json',
'{workspaceRoot}/apps/third-app/.eslintrc.json',
'{workspaceRoot}/tools/eslint-rules/**/*',
{ externalDependencies: ['eslint'] },
],
options: { cwd: 'apps/third-app' },
configurations: {},
command: 'eslint .',
}
);
expect(merged).toMatchInlineSnapshot(`
{
"cache": true,
"command": "eslint .",
"configurations": {},
"inputs": [
"default",
"{workspaceRoot}/.eslintrc.json",
"{workspaceRoot}/apps/third-app/.eslintrc.json",
"{workspaceRoot}/tools/eslint-rules/**/*",
{
"externalDependencies": [
"eslint",
],
},
],
"options": {
"command": "eslint . -o outputfile.json",
"cwd": "apps/third-app",
},
"outputs": [
"{projectRoot}/outputfile.json",
],
}
`);
});
});
});

describe('mergeProjectConfigurationIntoRootMap', () => {
Expand Down Expand Up @@ -1081,18 +1181,10 @@ describe('project-configuration-utils', () => {
expect(
isCompatibleTarget(
{
command: 'echo',
},
{
command: 'echo',
}
)
).toBe(true);

expect(
isCompatibleTarget(
{
command: 'echo',
executor: 'nx:run-commands',
options: {
command: 'echo',
},
},
{
executor: 'nx:run-commands',
Expand All @@ -1108,18 +1200,10 @@ describe('project-configuration-utils', () => {
expect(
isCompatibleTarget(
{
command: 'echo',
},
{
command: 'echo2',
}
)
).toBe(false);

expect(
isCompatibleTarget(
{
command: 'echo',
executor: 'nx:run-commands',
options: {
command: 'echo',
},
},
{
executor: 'nx:run-commands',
Expand All @@ -1130,19 +1214,6 @@ describe('project-configuration-utils', () => {
)
).toBe(false);
});

it('should return false if one target specifies an executor and the other a command', () => {
expect(
isCompatibleTarget(
{
executor: 'nx:noop',
},
{
command: 'echo',
}
)
).toBe(false);
});
});
});

Expand Down
106 changes: 72 additions & 34 deletions packages/nx/src/project-graph/utils/project-configuration-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,36 +133,37 @@ export function mergeProjectConfigurationIntoRootMap(
updatedProjectConfiguration.targets = matchingProject?.targets ?? {};

// For each target defined in the new config
for (const target in project.targets) {
for (const targetName in project.targets) {
// Always set source map info for the target, but don't overwrite info already there
// if augmenting an existing target.
if (
sourceMap &&
!project.targets[target]?.[ONLY_MODIFIES_EXISTING_TARGET]
) {
sourceMap[`targets.${target}`] = sourceInformation;

const target = project.targets?.[targetName];

if (sourceMap && !target?.[ONLY_MODIFIES_EXISTING_TARGET]) {
sourceMap[`targets.${targetName}`] = sourceInformation;
}

// If ONLY_MODIFIES_EXISTING_TARGET is true, and its not on the matching project
// we shouldn't merge its info into the graph
if (
project.targets[target]?.[ONLY_MODIFIES_EXISTING_TARGET] &&
!matchingProject.targets?.[target]
target?.[ONLY_MODIFIES_EXISTING_TARGET] &&
!matchingProject.targets?.[targetName]
) {
continue;
}

// We don't want the symbol to live on past the merge process
if (project.targets[target]?.[ONLY_MODIFIES_EXISTING_TARGET])
delete project.targets[target]?.[ONLY_MODIFIES_EXISTING_TARGET];

updatedProjectConfiguration.targets[target] = mergeTargetConfigurations(
project.targets[target],
matchingProject.targets?.[target],
sourceMap,
sourceInformation,
`targets.${target}`
);
if (target?.[ONLY_MODIFIES_EXISTING_TARGET])
delete target?.[ONLY_MODIFIES_EXISTING_TARGET];

updatedProjectConfiguration.targets[targetName] =
mergeTargetConfigurations(
resolveCommandSyntacticSugar(target, project.root),
matchingProject.targets?.[targetName],
sourceMap,
sourceInformation,
`targets.${target}`
);
}
}

Expand Down Expand Up @@ -487,25 +488,35 @@ export function isCompatibleTarget(
a: TargetConfiguration,
b: TargetConfiguration
) {
if (a.command || b.command) {
const aCommand =
a.command ??
(a.executor === 'nx:run-commands' ? a.options?.command : null);
const bCommand =
b.command ??
(b.executor === 'nx:run-commands' ? b.options?.command : null);

const sameCommand = aCommand === bCommand;
const aHasNoExecutor = !a.command && !a.executor;
const bHasNoExecutor = !b.command && !b.executor;

return sameCommand || aHasNoExecutor || bHasNoExecutor;
}

const oneHasNoExecutor = !a.executor || !b.executor;
const bothHaveSameExecutor = a.executor === b.executor;

return oneHasNoExecutor || bothHaveSameExecutor;
if (oneHasNoExecutor) return true;
if (!bothHaveSameExecutor) return false;

const isRunCommands = a.executor === 'nx:run-commands';
if (isRunCommands) {
const aCommand = a.options?.command ?? a.options?.commands.join(' && ');
const bCommand = b.options?.command ?? b.options?.commands.join(' && ');

const oneHasNoCommand = !aCommand || !bCommand;
const hasSameCommand = aCommand === bCommand;

return oneHasNoCommand || hasSameCommand;
}

const isRunScript = a.executor === 'nx:run-script';
if (isRunScript) {
const aScript = a.options?.script;
const bScript = b.options?.script;

const oneHasNoScript = !aScript || !bScript;
const hasSameScript = aScript === bScript;

return oneHasNoScript || hasSameScript;
}

return true;
}

function mergeConfigurations<T extends Object>(
Expand Down Expand Up @@ -616,10 +627,37 @@ export function readTargetDefaultsForTarget(
return targetDefaults?.[targetName];
}
}

function createRootMap(projectRootMap: Map<string, ProjectConfiguration>) {
const map: Record<string, string> = {};
for (const [projectRoot, { name: projectName }] of projectRootMap) {
map[projectRoot] = projectName;
}
return map;
}

function resolveCommandSyntacticSugar(
target: TargetConfiguration,
key: string
): TargetConfiguration {
const { command, ...config } = target ?? {};

if (!command) {
return target;
}

if (config.executor) {
throw new Error(
`${NX_PREFIX} Project at ${key} should not have executor and command both configured.`
);
} else {
return {
...config,
executor: 'nx:run-commands',
options: {
...config.options,
command: command,
},
};
}
}

0 comments on commit 1ab03ad

Please sign in to comment.