Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): enforce fixed versions only for apps with createPackageJson #16398

Merged
merged 1 commit into from
Apr 20, 2023
Merged

fix(core): enforce fixed versions only for apps with createPackageJson #16398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/js/src/utils/package-json/update-package-json.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,8 @@ describe('updatePackageJson', () => {
expect(distPackageJson).toMatchInlineSnapshot(`
{
"dependencies": {
"external1": "1.0.0",
"external2": "4.5.6",
"external1": "~1.0.0",
"external2": "^4.0.0",
},
"main": "./main.js",
"name": "@org/lib1",
Expand Down
216 changes: 215 additions & 1 deletion packages/nx/src/plugins/js/package-json/create-package-json.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import * as fs from 'fs';

import * as configModule from '../../../config/configuration';
import { DependencyType } from '../../../config/project-graph';
import { DependencyType, ProjectGraph } from '../../../config/project-graph';
import * as hashModule from '../../../hasher/hasher';
import { createPackageJson } from './create-package-json';
import * as fileutilsModule from '../../../utils/fileutils';

jest.mock('../../../utils/workspace-root', () => ({
workspaceRoot: '/root',
}));

describe('createPackageJson', () => {
it('should add additional dependencies', () => {
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
Expand Down Expand Up @@ -499,4 +503,214 @@ describe('createPackageJson', () => {
);
expect(filterUsingGlobPatternsSpy).toHaveBeenCalledTimes(4);
});

describe('parsing "package.json" versions', () => {
const appDependencies = [
{ source: 'app1', target: 'npm:@nx/devkit', type: 'static' },
{ source: 'app1', target: 'npm:typescript', type: 'static' },
];

const libDependencies = [
{ source: 'lib1', target: 'npm:@nx/devkit', type: 'static' },
{ source: 'lib1', target: 'npm:tslib', type: 'static' },
{ source: 'lib1', target: 'npm:typescript', type: 'static' },
];

const graph: ProjectGraph = {
nodes: {
app1: {
type: 'app',
name: 'app1',
data: {
files: [
{
file: '/root/apps/app1/src/main.ts',
hash: '',
dependencies: appDependencies,
},
],
targets: {},
root: '/root/apps/app1',
},
},
lib1: {
type: 'lib',
name: 'lib1',
data: {
files: [
{
file: '/root/libs/lib1/index.ts',
hash: '',
dependencies: libDependencies,
},
],
targets: {},
root: '/root/libs/lib1',
},
},
},
externalNodes: {
'npm:@nx/devkit': {
type: 'npm',
name: 'npm:@nx/devkit',
data: { version: '16.0.0', hash: '', packageName: '@nx/devkit' },
},
'npm:nx': {
type: 'npm',
name: 'npm:nx',
data: { version: '16.0.0', hash: '', packageName: 'nx' },
},
'npm:tslib': {
type: 'npm',
name: 'npm:tslib',
data: { version: '2.4.4', hash: '', packageName: 'tslib' },
},
'npm:typescript': {
type: 'npm',
name: 'npm:typescript',
data: { version: '4.9.5', hash: '', packageName: 'typescript' },
},
},
dependencies: {
app1: appDependencies,
lib1: libDependencies,
},
};

const rootPackageJson = () => ({
dependencies: {
'@nx/devkit': '~16.0.0',
nx: '> 14',
typescript: '^4.8.2',
tslib: '~2.4.0',
},
});

const projectPackageJson = () => ({
name: 'other-name',
version: '1.2.3',
dependencies: {
typescript: '^4.8.4',
random: '1.0.0',
},
});

const spies = [];

afterEach(() => {
while (spies.length > 0) {
spies.pop().mockRestore();
}
});

it('should use fixed versions when creating package json for apps', () => {
spies.push(
jest
.spyOn(fileutilsModule, 'readJsonFile')
.mockImplementation((path) => {
if (path === '/root/package.json') {
return rootPackageJson();
}
})
);

expect(createPackageJson('app1', graph)).toEqual({
dependencies: {
'@nx/devkit': '16.0.0',
nx: '16.0.0',
typescript: '4.9.5',
},
name: 'app1',
version: '0.0.1',
});
});

it('should override fixed versions with local ranges when creating package json for apps', () => {
spies.push(
jest.spyOn(fs, 'existsSync').mockImplementation((path) => {
if (path === '/root/apps/app1/package.json') {
return true;
}
})
);
spies.push(
jest
.spyOn(fileutilsModule, 'readJsonFile')
.mockImplementation((path) => {
if (path === '/root/package.json') {
return rootPackageJson();
}
if (path === '/root/apps/app1/package.json') {
return projectPackageJson();
}
})
);

expect(createPackageJson('app1', graph)).toEqual({
dependencies: {
'@nx/devkit': '16.0.0',
nx: '16.0.0',
random: '1.0.0',
typescript: '^4.8.4',
},
name: 'other-name',
version: '1.2.3',
});
});

it('should use range versions when creating package json for libs', () => {
spies.push(
jest
.spyOn(fileutilsModule, 'readJsonFile')
.mockImplementation((path) => {
if (path === '/root/package.json') {
return rootPackageJson();
}
})
);

expect(createPackageJson('lib1', graph)).toEqual({
dependencies: {
'@nx/devkit': '~16.0.0',
tslib: '~2.4.0',
typescript: '^4.8.2',
},
name: 'lib1',
version: '0.0.1',
});
});

it('should override range versions with local ranges when creating package json for libs', () => {
spies.push(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of having to mock these functions, but since it's used in existing tests let's go with it for now. Maybe something we can discuss later is testing strategies

jest.spyOn(fs, 'existsSync').mockImplementation((path) => {
if (path === '/root/libs/lib1/package.json') {
return true;
}
})
);
spies.push(
jest
.spyOn(fileutilsModule, 'readJsonFile')
.mockImplementation((path) => {
if (path === '/root/package.json') {
return rootPackageJson();
}
if (path === '/root/libs/lib1/package.json') {
return projectPackageJson();
}
})
);

expect(createPackageJson('lib1', graph)).toEqual({
dependencies: {
'@nx/devkit': '~16.0.0',
random: '1.0.0',
tslib: '~2.4.0',
typescript: '^4.8.4',
},
name: 'other-name',
version: '1.2.3',
});
});
});
});
89 changes: 61 additions & 28 deletions packages/nx/src/plugins/js/package-json/create-package-json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export function createPackageJson(
} = {}
): PackageJson {
const projectNode = graph.nodes[projectName];
const isLibrary = projectNode.type === 'lib';

const { selfInputs, dependencyInputs } = options.target
? getTargetInputs(readNxJson(), projectNode, options.target)
Expand Down Expand Up @@ -91,6 +92,18 @@ export function createPackageJson(
} catch (e) {}
}

const getVersion = (
packageName,
version,
section: 'devDependencies' | 'dependencies'
) => {
return (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a nitpick, but IMO it's more clear like this:

return isLibrary
  ? packageJson[section][packageName] ||
      rootPackageJson[section]?.[packageName] ||
      version
  : packageJson[section][packageName] || version;

Or this?

if (isLibrary) 
  return packageJson[section][packageName] ||
      rootPackageJson[section]?.[packageName] ||
      version;
else
  return packageJson[section][packageName] || version;

Reasoning is that having isLibrary in the middle is harder to pick up at a quick glance. Putting it at the top separate the check for lib vs app, and finding default versions.

packageJson[section][packageName] ||
(isLibrary && rootPackageJson[section]?.[packageName]) ||
version
);
};

const rootPackageJson = readJsonFile(
`${options.root || workspaceRoot}/package.json`
);
Expand All @@ -103,44 +116,64 @@ export function createPackageJson(
// don't store dev dependencies for production
if (!options.isProduction) {
packageJson.devDependencies ??= {};
packageJson.devDependencies[packageName] = version;
packageJson.devDependencies[packageName] = getVersion(
packageName,
version,
'devDependencies'
);
}
} else {
if (!packageJson.peerDependencies?.[packageName]) {
packageJson.dependencies ??= {};
packageJson.dependencies[packageName] = version;
packageJson.dependencies[packageName] = getVersion(
packageName,
version,
'dependencies'
);
}
}
});
Object.entries(npmDeps.peerDependencies).forEach(([packageName, version]) => {
if (!packageJson.peerDependencies?.[packageName]) {
if (rootPackageJson.dependencies?.[packageName]) {
packageJson.dependencies ??= {};
packageJson.dependencies[packageName] = version;
return;
}
if (!isLibrary) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now, but if the app vs lib logic deviates more, we might consider separating them into two functions: createAppPackageJson and createLibPackgeJson so the consumer can choose which to call depending on use-case.

Object.entries(npmDeps.peerDependencies).forEach(
([packageName, version]) => {
if (!packageJson.peerDependencies?.[packageName]) {
if (rootPackageJson.dependencies?.[packageName]) {
packageJson.dependencies ??= {};
packageJson.dependencies[packageName] = getVersion(
packageName,
version,
'dependencies'
);
return;
}

const isOptionalPeer =
npmDeps.peerDependenciesMeta[packageName]?.optional;
if (!isOptionalPeer) {
if (
!options.isProduction ||
rootPackageJson.dependencies?.[packageName]
) {
packageJson.peerDependencies ??= {};
packageJson.peerDependencies[packageName] = version;
const isOptionalPeer =
npmDeps.peerDependenciesMeta[packageName]?.optional;
if (!isOptionalPeer) {
if (
!options.isProduction ||
rootPackageJson.dependencies?.[packageName]
) {
packageJson.peerDependencies ??= {};
packageJson.peerDependencies[packageName] = getVersion(
packageName,
version,
'dependencies'
);
}
} else if (!options.isProduction) {
// add peer optional dependencies if not in production
packageJson.peerDependencies ??= {};
packageJson.peerDependencies[packageName] = version;
packageJson.peerDependenciesMeta ??= {};
packageJson.peerDependenciesMeta[packageName] = {
optional: true,
};
}
}
} else if (!options.isProduction) {
// add peer optional dependencies if not in production
packageJson.peerDependencies ??= {};
packageJson.peerDependencies[packageName] = version;
packageJson.peerDependenciesMeta ??= {};
packageJson.peerDependenciesMeta[packageName] = {
optional: true,
};
}
}
});
);
}

packageJson.devDependencies &&= sortObjectByKeys(packageJson.devDependencies);
packageJson.dependencies &&= sortObjectByKeys(packageJson.dependencies);
Expand Down