-
Notifications
You must be signed in to change notification settings - Fork 607
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
Replace install-test-workspace with usePnpmSyncForInjectedDependencies=true #4530
Conversation
052621a
to
58b0d61
Compare
common/changes/@microsoft/rush/chao-pnpm-sync_2024-02-21-20-27.json
Outdated
Show resolved
Hide resolved
ed75930
to
d4c81d4
Compare
The new projects should have build cache configured. |
22ea872
to
3850f51
Compare
6161fc1
to
61febbe
Compare
380523f
to
f95f5c1
Compare
@@ -129,6 +129,10 @@ These GitHub repositories provide supplementary resources for Rush Stack: | |||
| [/build-tests-samples/heft-web-rig-library-tutorial](./build-tests-samples/heft-web-rig-library-tutorial/) | (Copy of sample project) Building this project is a regression test for Heft | |
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.
It can be fixed in a separate PR, but this log notice can be improved.
-
Like the build cache notices, it should only be shown when invoking
rush build --verbose
. -
Also I would suggest to improve the formatting of the message:
pnpm-sync: Copied 107 files in 95ms from C:\Git\rushstack\libraries\terminal
-
It seems
pnpmSyncCopy()
is printing this message directly to the console. We should improve the API to provide a messaging callback that allows the output to be redirected and (ideally) optionally reformatted. Something like this:pnpmSyncCopy({ pnpmSyncJsonPath, messageCallback: ({ message, messageKind: 'error'|'warning'|'info'|'verbose'|'timing', messageId, details }) => { if (messageKind === 'verbose' && !debug) { return; } switch (messageId) { case 'sync-finished': // customized logging; the structure of details can depend on messageId terminal.writeLine(colors.green('pnpm-sync') + ` copied ${details.fileCount} files in ${details.totalMs} ms from ${details.sourcePath}`); break; default: // simple preformatted logging if (messageKind === 'error' | messageKind === 'warning') { console.error(message); } else { console.log(message); } break; } } })
This would ensure that Rush (and in the future PNPM) has full ownership of its CLI UX.
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.
Summary
Before, the
install-test-worspace
is a standalone pnpm workspace project inside Rushstack.Now, we have integrated the pnpm-sync feature, so we can refactor this project to use
pnpm-sync
to manage injected dependencies.Details
usePnpmSyncForInjectedDependencies
inexperiments.json
install-test-worspace
folders into a new Rush category folderbuild-tests-subspace
👉 This PR introduces injected dependencies but not subspaces yet. The folder is named
build-tests-subspace
because a future PR will enable the Rush subspaces feature to reintroduce a separate lockfile for these projects.How it was tested
rush
:rush build
console.log("TEST 1")
to rush-lib/src/index.tsrush build --to rush-lib-test --verbose
cd rush-lib-test && node lib/start.js
printsTEST 1
, showing the updated files were copied successfullyrushx
:console.log("TEST 2")
to rush-lib/src/index.tscd rush-lib && rushx build
cd rush-lib-test && node lib/start.js
printsTEST 2
, showing the updated files were again copied successfullyImpacted documentation
N/A