fix(node-packages): pin peer dependencies with undefined version #1609
Conversation
test/javascript/node-package.test.ts
Outdated
| expect(pkgFile.peerDependencies).toStrictEqual({ ms: "^2.1.3" }); | ||
| expect(pkgFile.devDependencies).toStrictEqual({ ms: "2.1.3" }); |
There was a problem hiding this comment.
Let's change this to some other random version number to make it clear that the mock is working and the test isn't dependent on the actual latest version of "ms"
test/javascript/node-package.test.ts
Outdated
| const project = new Project({ name: "test" }); | ||
| const pkg = new NodePackage(project); | ||
|
|
||
| const orig = { | ||
| name: "test", | ||
| devDependencies: { | ||
| ms: "^2.2.2", | ||
| }, | ||
| main: "lib/index.js", | ||
| license: "Apache-2.0", | ||
| version: "0.0.0", | ||
| "//": '~~ Generated by projen. To modify, edit .projenrc.js and run "npx projen".', | ||
| }; |
There was a problem hiding this comment.
I'm not sure if I understand this test case. It looks like we are asserting that if the user happens to have "ms" as a dev dependency (but not configured by projen), that projen will not change that after synthesizing? I'm not sure if that is an appropriate promise to make in our API -- either the user should be managing all of their dependencies through projen, or none of them.
There was a problem hiding this comment.
This was included to mimic the current behaviour, on line 979 of node-package.ts:
// respect user version if it's not '*' or if current version is undefined
I'd be more than happy for the behaviour of the peer pinning to behave differently in this case if that's preferable?
test/javascript/node-package.test.ts
Outdated
| const project = new Project({ name: "test" }); | ||
| const pkg = new NodePackage(project); |
There was a problem hiding this comment.
If these test cases are assuming that pinnedDevDependencies is true, we should specify that explicitly in the tests when initializing NodePackage, and not take the default for granted. If the test case assertions here are irrespective of the setting of pinnedDevDependencies, then I think it might make sense to show that explicitly (e.g. by dividing this into one test where pinnedDevDependencies is true, and one test where it's false).
Codecov Report
@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
+ Coverage 88.06% 89.38% +1.31%
==========================================
Files 132 146 +14
Lines 5109 5970 +861
Branches 1207 1543 +336
==========================================
+ Hits 4499 5336 +837
- Misses 610 632 +22
- Partials 0 2 +2
Continue to review full report at Codecov.
|
|
I've raised an issue and linked to it in the PR description, and modified the tests.
|
There was a problem hiding this comment.
Thanks for your patience @cgatt. I think is almost ready to get merged in, just have a few comments to improve readability in a few places.
Overall the code in projen for resolving dependencies is a bit confusing to grok / feels like it could benefit from some kind of refactoring, but that makes more sense for a separate PR. I appreciate you writing out several test cases and keeping the actual fix here pretty self-contained. 👍
Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>
Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>
fixes #1611
Peer dependencies added without a fixed version are being added to devDependencies in package.json as
name@^versioneven withpinnedDevDependency=true, which is throwing a JSII warning. This change adds another pass through the peer dependencies in the post-synth step to pin those peer dependencies who's version is not known during presynth. While this causes a degree of code duplication, I decided against replacing the pre-synth peerDependency pass as it minimises the impact of this change on existing workflows.This change is tested both in mocked unit tests and through yarn link to a local project, where it displayed the expected behaviour, including correctly respecting any existing versioning in
package.json.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.