fix(sys): make NodeLazyRequire complain if package versions aren't right #3346

alicewriteswrongs · 2022-04-26T17:05:20Z

This updates a bit of logic in NodeLazyRequire.ensure to check that
the installed versions of packages are within the specified version
range, i.e. that minVersion <= installedVersion <= maxVersion.

This commit also adds tests for that module.

STENCIL-391: bug: @stencil/core does not throw error when missing
jest/jest-cli deps in a rush/pnpm monorepo

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

GitHub Issue Number: N/A

If a stencil user has a package installed that's out of the range we current don't show an error, so we could end up just having runtime errors relating to unsupported differences in those packages.

What is the new behavior?

Now if the user does not have a version in the range we log an error instead!

Does this introduce a breaking change?

Yes
No

Testing

I wrote some tests, and tried out the behavior locally with a little test component.

rwaskiewicz

Implementation looks good to me! Just a few asks for tests

rwaskiewicz · 2022-04-26T18:36:38Z

src/sys/node/test/node-lazy-require.spec.ts

+        header: 'Please install supported versions of dev dependencies with either npm or yarn.',
+        messageText: 'npm install --save-dev jest@38.0.1',
+      });
+    });


Could we add one more test here to verify that this returns an error for prereleases? E.g.

Suggested change

});

});

it('should error if the installed version of a package is too high (prereleases)', async () => {

const { nodeLazyRequire, readFSMock } = setup();

readFSMock.mockReturnValue(mockPackageJson('38.0.1-alpha.0'));

let [error] = await nodeLazyRequire.ensure('.', ['jest']);

expect(error).toEqual({

...buildError([]),

header: 'Please install supported versions of dev dependencies with either npm or yarn.',

messageText: 'npm install --save-dev jest@38.0.1',

});

});

makes sense - I parameterized this test and added that version string, and then also added a few more to the other test which tests what should be accepted as valid versions — 3bd58e2

rwaskiewicz · 2022-04-26T18:40:45Z

src/sys/node/test/node-lazy-require.spec.ts

+
+describe('node-lazy-require', () => {
+  describe('NodeLazyRequire', () => {
+    function setup() {


since we're testing the ensure method on NodeLazyRequire, can we add a suite that wraps all this setup/tests?

describe('node-lazy-require', () => { describe('NodeLazyRequire', () => { describe('ensure', () => { function setup() { .. } ... it('should error if the installed version of a package is too high', async () => { ... }); }); }); }); });

sure, makes sense to me — 7414f82

rwaskiewicz · 2022-04-26T18:45:15Z

src/sys/node/test/node-lazy-require.spec.ts

+  describe('NodeLazyRequire', () => {
+    function setup() {
+      const resolveModule = new NodeResolveModule();
+      const readFSMock = jest.spyOn(fs, 'readFileSync').mockReturnValue(mockPackageJson('10.10.10'));


Can we make sure we clear this mock between tests? With the current setup, I think we're generally OK, in that we call setup in each test, but I think might be a good idea to clean up in an afterEach as well to be explicit about reseting things

rwaskiewicz · 2022-04-26T19:00:13Z

src/sys/node/node-lazy-require.ts

-    private nodeResolveModule: NodeResolveModule,
-    private lazyDependencies: { [dep: string]: [string, string] }
-  ) {}
+  constructor(private nodeResolveModule: NodeResolveModule, private lazyDependencies: LazyDependencies) {}

  async ensure(fromDir: string, ensureModuleIds: string[]) {


Minor, can we JSDoc this function while we're in here?

addressed in e452f2a

Ahh sorry if I wasn't clear here - meant on the ensure() method since we were getting it under test. The ctor JSDoc looks good though! Let's keep that 🙂

no worries! just added that in 8c90322

alicewriteswrongs · 2022-04-26T19:42:56Z

package.json

@@ -119,7 +119,6 @@
    "puppeteer": "~10.0.0",
    "rollup": "2.42.3",
    "rollup-plugin-sourcemaps": "^0.6.3",
-    "semiver": "^1.1.0",


I removed this as a dependency since we were only using it in a few places and already depending on semver.

I noticed references to this package in scripts/license.ts and NOTICE.md — is there any manual intervention needed to sort those out?

possible nvm on this one - found scripts/license.ts and ran it, see 684fe50

alicewriteswrongs · 2022-04-26T19:49:52Z

@rwaskiewicz I think I addressed all your comments

rwaskiewicz · 2022-04-26T20:50:46Z

src/sys/node/test/node-lazy-require.spec.ts

+describe('node-lazy-require', () => {
+  describe('NodeLazyRequire', () => {
+    describe('ensure', () => {
+      let readFSMock: jest.SpyInstance;


Minor suggestion - can we add types to the spy here?

Suggested change

let readFSMock: jest.SpyInstance;

let readFSMock: jest.SpyInstance<ReturnType<typeof fs.readFileSync>, Parameters<typeof fs.readFileSync>>;

rwaskiewicz · 2022-04-26T20:59:38Z

src/sys/node/node-lazy-require.ts

-    private nodeResolveModule: NodeResolveModule,
-    private lazyDependencies: { [dep: string]: [string, string] }
-  ) {}
+  constructor(private nodeResolveModule: NodeResolveModule, private lazyDependencies: LazyDependencies) {}

  async ensure(fromDir: string, ensureModuleIds: string[]) {


Ahh sorry if I wasn't clear here - meant on the ensure() method since we were getting it under test. The ctor JSDoc looks good though! Let's keep that 🙂

This updates a bit of logic in `NodeLazyRequire.ensure` to check that the installed versions of packages are within the specified version range, i.e. that `minVersion <= installedVersion <= maxVersion`. This commit also adds tests for that module. STENCIL-391: bug: @stencil/core does not throw error when missing jest/jest-cli deps in a rush/pnpm monorepo

alicewriteswrongs requested a review from a team April 26, 2022 17:05

alicewriteswrongs force-pushed the ap/check-lazy-version-not-too-high branch 2 times, most recently from 25359e6 to 24eb9d2 Compare April 26, 2022 17:10

rwaskiewicz suggested changes Apr 26, 2022

View reviewed changes

alicewriteswrongs commented Apr 26, 2022

View reviewed changes

rwaskiewicz reviewed Apr 26, 2022

View reviewed changes

alicewriteswrongs added 8 commits April 27, 2022 17:39

constructor

da61eb2

another describe block

382bb75

reset mock, move that part to (before|after)Each

4f053a3

add some more cases for parameterized tests

b1b8f20

license thing

cd403d4

add a JSDoc for NodeLazyRequire.ensure

5316818

update the spy type

8c71d85

alicewriteswrongs force-pushed the ap/check-lazy-version-not-too-high branch from 8c90322 to 8c71d85 Compare April 27, 2022 21:39

rwaskiewicz approved these changes Apr 28, 2022

View reviewed changes

rwaskiewicz merged commit b7adc33 into main Apr 28, 2022

rwaskiewicz deleted the ap/check-lazy-version-not-too-high branch April 28, 2022 12:39

alicewriteswrongs mentioned this pull request Apr 28, 2022

fix(sys): tweak NodeLazyRequire logic around too-high-versions errors #3347

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sys): make NodeLazyRequire complain if package versions aren't right #3346

fix(sys): make NodeLazyRequire complain if package versions aren't right #3346

alicewriteswrongs commented Apr 26, 2022 •

edited

Loading

rwaskiewicz left a comment

rwaskiewicz Apr 26, 2022

alicewriteswrongs Apr 26, 2022 •

edited

Loading

rwaskiewicz Apr 26, 2022

alicewriteswrongs Apr 26, 2022

rwaskiewicz Apr 26, 2022

alicewriteswrongs Apr 26, 2022

rwaskiewicz Apr 26, 2022

alicewriteswrongs Apr 26, 2022

rwaskiewicz Apr 26, 2022

alicewriteswrongs Apr 27, 2022

alicewriteswrongs Apr 26, 2022

alicewriteswrongs Apr 26, 2022

alicewriteswrongs commented Apr 26, 2022

rwaskiewicz Apr 26, 2022 •

edited

Loading

rwaskiewicz Apr 26, 2022

-    });
+    });
+    it('should error if the installed version of a package is too high (prereleases)', async () => {
+      const { nodeLazyRequire, readFSMock } = setup();
+      readFSMock.mockReturnValue(mockPackageJson('38.0.1-alpha.0'));
+      let [error] = await nodeLazyRequire.ensure('.', ['jest']);
+      expect(error).toEqual({
+        ...buildError([]),
+        header: 'Please install supported versions of dev dependencies with either npm or yarn.',
+        messageText: 'npm install --save-dev jest@38.0.1',
+      });
+    });

	let readFSMock: jest.SpyInstance;
	let readFSMock: jest.SpyInstance<ReturnType<typeof fs.readFileSync>, Parameters<typeof fs.readFileSync>>;

fix(sys): make NodeLazyRequire complain if package versions aren't right #3346

fix(sys): make NodeLazyRequire complain if package versions aren't right #3346

Conversation

alicewriteswrongs commented Apr 26, 2022 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 26, 2022

rwaskiewicz Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 26, 2022 •

edited

Loading

alicewriteswrongs Apr 26, 2022 •

edited

Loading

rwaskiewicz Apr 26, 2022 •

edited

Loading