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.

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

chore(repo): add conformance rule for our package.json files #29078

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion nx.json
Original file line number Diff line number Diff line change
Expand Up @@ -247,5 +247,12 @@
"parallel": 1,
"cacheDirectory": "/tmp/nx-cache",
"bust": 1,
"defaultBase": "master"
"defaultBase": "master",
"conformance": {
"rules": [
{
"rule": "@nx/workspace-plugin/conformance-rules/project-package-json"
}
]
}
}
20 changes: 10 additions & 10 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@
"@eslint/eslintrc": "^2.1.1",
"@eslint/js": "^8.48.0",
"@floating-ui/react": "0.26.6",
"@jest/reporters": "^29.4.1",
"@jest/test-result": "^29.4.1",
"@jest/types": "^29.4.1",
"@jest/reporters": "29.7.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our jest packages weren't aligned for some reason

"@jest/test-result": "29.7.0",
"@jest/types": "29.6.3",
"@module-federation/enhanced": "0.7.6",
"@module-federation/sdk": "0.7.6",
"@monodon/rust": "2.1.1",
Expand Down Expand Up @@ -83,11 +83,11 @@
"@nx/powerpack-enterprise-cloud": "1.1.0-beta.5",
"@nx/powerpack-license": "1.1.0-beta.5",
"@nx/react": "20.2.0-beta.3",
"@nx/rspack": "20.2.0-beta.3",
"@nx/storybook": "20.2.0-beta.3",
"@nx/vite": "20.2.0-beta.3",
"@nx/web": "20.2.0-beta.3",
"@nx/webpack": "20.2.0-beta.3",
"@nx/rspack": "20.2.0-beta.3",
"@phenomnomnominal/tsquery": "~5.0.1",
"@playwright/test": "^1.36.1",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.7",
Expand Down Expand Up @@ -217,13 +217,13 @@
"jasmine-core": "~2.99.1",
"jasmine-spec-reporter": "~4.2.1",
"jest": "29.7.0",
"jest-config": "^29.4.1",
"jest-diff": "^29.4.1",
"jest-config": "29.7.0",
"jest-diff": "29.7.0",
"jest-environment-jsdom": "29.7.0",
"jest-environment-node": "^29.4.1",
"jest-resolve": "^29.4.1",
"jest-runtime": "^29.4.1",
"jest-util": "^29.4.1",
"jest-environment-node": "29.7.0",
"jest-resolve": "29.7.0",
"jest-runtime": "29.7.0",
"jest-util": "29.7.0",
"js-tokens": "^4.0.0",
"jsonc-eslint-parser": "^2.1.0",
"jsonc-parser": "3.2.0",
Expand Down
3 changes: 3 additions & 0 deletions packages/rspack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,8 @@
},
"nx-migrations": {
"migrations": "./migrations.json"
},
"publishConfig": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found by the new rule!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we needed this for provenance... how does rspack have provenance without this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not for provenance, it's needed because the default access for new scoped package is restricted and so the first time we publish a new package we need this explicit config. Technically after that point we no longer need it anymore, but it doesn't do any harm, just makes its state explicit, and so having this be a rule for all package.json files means we won't run into any issues when new packages are added in future.

"access": "public"
}
}
18 changes: 9 additions & 9 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion tools/workspace-plugin/jest.config.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/* eslint-disable */
export default {
displayName: 'workspace-plugin',
preset: '../../jest.preset.js',
// TODO: For some reason our patched jest resolve cannot work with @nx/powerpack-conformance
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needs some time spent on it, there's a lot of custom complexity there, the default resolver works fine...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this is blocking the PR after all because the existing code in this library needs the custom resolver, damn

// preset: '../../jest.preset.js',
transform: {
'^.+\\.[tj]s$': ['ts-jest', { tsconfig: '<rootDir>/tsconfig.spec.json' }],
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
const mockExistsSync = jest.fn();
jest.mock('node:fs', () => {
return {
...jest.requireActual('node:fs'),
existsSync: mockExistsSync,
};
});

import { validateProjectPackageJson } from './index';

const VALID_PACKAGE_JSON_BASE = {
name: '@nx/test-project',
publishConfig: {
access: 'public',
},
};

describe('project-package-json', () => {
afterEach(() => {
jest.resetAllMocks();
});

// Unit test the core implementation details of validating the project package.json
describe('validateProjectPackageJson()', () => {
it('should return no violations for a valid project package.json', () => {
const packageJson = {
...VALID_PACKAGE_JSON_BASE,
};
const sourceProject = 'test-project';
const sourceProjectRoot = '/path/to/test-project';
const violations = validateProjectPackageJson(
packageJson,
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
);
expect(violations).toEqual([]);
});

it('should return a violation if the name is not a string', () => {
const packageJson = {
...VALID_PACKAGE_JSON_BASE,
};
delete packageJson.name;

const sourceProject = 'test-project';
const sourceProjectRoot = '/path/to/test-project';
const violations = validateProjectPackageJson(
packageJson,
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
);
expect(violations).toMatchInlineSnapshot(`
[
{
"file": "/path/to/test-project/package.json",
"message": "The project package.json should have a "name" field",
"sourceProject": "test-project",
},
]
`);
});

it('should return a violation if the name is not scoped an org that is not @nx', () => {
const sourceProject = 'test-project';
const sourceProjectRoot = '/path/to/test-project';

expect(
validateProjectPackageJson(
// Should be fine, as not scoped
{
...VALID_PACKAGE_JSON_BASE,
name: 'test-project',
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// Should return a violation, as scoped to an org that is not @nx
const packageJsonWithScope = {
...VALID_PACKAGE_JSON_BASE,
name: '@nx-labs/test-project',
};
expect(
validateProjectPackageJson(
packageJsonWithScope,
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toMatchInlineSnapshot(`
[
{
"file": "/path/to/test-project/package.json",
"message": "The package name should be scoped to the @nx org",
"sourceProject": "test-project",
},
]
`);
});

it('should return a violation if a public package does not have publishConfig.access set to public', () => {
const sourceProject = 'some-project-name';
const sourceProjectRoot = '/path/to/some-project-name';

expect(
validateProjectPackageJson(
// Should be fine, as private
{
private: true,
name: 'test-project',
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// Should return a violation, as not private
const packageJsonWithoutPublicAccess = {
...VALID_PACKAGE_JSON_BASE,
};
delete packageJsonWithoutPublicAccess.publishConfig;
expect(
validateProjectPackageJson(
packageJsonWithoutPublicAccess,
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toMatchInlineSnapshot(`
[
{
"file": "/path/to/some-project-name/package.json",
"message": "Public packages should have "publishConfig": { "access": "public" } set in their package.json",
"sourceProject": "some-project-name",
},
]
`);
});

it('should return a violation if the project has an executors.json but does not reference it in the package.json', () => {
const sourceProject = 'some-project-name';
const sourceProjectRoot = '/path/to/some-project-name';

// The project does not have an executors.json, so no violation
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// The project has an executors.json
mockExistsSync.mockImplementation((path) => {
if (path.endsWith('executors.json')) {
return true;
}
return false;
});

// The project references the executors.json in the package.json, so no violation
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
executors: './executors.json',
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// The project does not reference the executors.json in the package.json, so a violation is returned
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toMatchInlineSnapshot(`
[
{
"file": "/path/to/some-project-name/package.json",
"message": "The project has an executors.json, but does not reference "./executors.json" in the "executors" field of its package.json",
"sourceProject": "some-project-name",
},
]
`);
});

it('should return a violation if the project has an generators.json but does not reference it in the package.json', () => {
const sourceProject = 'some-project-name';
const sourceProjectRoot = '/path/to/some-project-name';

// The project does not have an generators.json, so no violation
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// The project has an generators.json
mockExistsSync.mockImplementation((path) => {
if (path.endsWith('generators.json')) {
return true;
}
return false;
});

// The project references the generators.json in the package.json, so no violation
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
generators: './generators.json',
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toEqual([]);

// The project does not reference the generators.json in the package.json, so a violation is returned
expect(
validateProjectPackageJson(
{
...VALID_PACKAGE_JSON_BASE,
},
sourceProject,
sourceProjectRoot,
`${sourceProjectRoot}/package.json`
)
).toMatchInlineSnapshot(`
[
{
"file": "/path/to/some-project-name/package.json",
"message": "The project has an generators.json, but does not reference "./generators.json" in the "generators" field of its package.json",
"sourceProject": "some-project-name",
},
]
`);
});
});
});
Loading