-
Notifications
You must be signed in to change notification settings - Fork 370
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
feat(deps): refactor node-workspace plugin to drop lerna-lite/core #2117
Conversation
test: fix test
Use the internal workspace-plugin graph/dependency logic and built-in package.json updaters to update the content.
15cbb65
to
35c6fd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few questions.
@@ -157,7 +189,7 @@ describe('NodeWorkspace plugin', () => { | |||
), | |||
], | |||
}), | |||
buildMockCandidatePullRequest('node1', 'node', '2.2.2', { | |||
buildMockCandidatePullRequest('node1', 'node', '3.3.3', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the mock values need to change? It would be good to see the same tests running with the same assertions against the refactored code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node1 package (@here/pkgA
) fixture is already at 3.3.3. The lerna updater only updated the dependency versions, so the output still had 3.3.3 as the @here/pkgA
version. But switching to the PackageJson updater, it actually did change the value from 3.3.3 to 2.2.2 (which was invalid). To keep the test output the same before and after the refactor, I switched this value to be 3.3.3
.
snapshot(dateSafe(nodeCandidate!.pullRequest.body.toString())); | ||
snapshotUpdate(updates, 'package.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why this order changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snapshot name is based on an incrementing count. To make the snapshots only additive (and easier to review), I put these ones after the existing snapshot.
], | ||
"author": "Ben Coe <[email protected]>", | ||
"license": "ISC", | ||
"devDependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you tested very type of dependency here (dev, optional, peer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to eliminate a dependency, good work.
Refactors the node-workspace plugin to use the internal workspace-plugin graph/dependency logic and built-in
package.json updaters to update the content. We do not need to rely on lerna internals.
Fixes #2116
feat: PackageJson updater can update dependencies