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
fix(bootstrap): respect --force-local option #2104
Conversation
Before this change, `--force-local` option was not respected and monorepo-local packages outside of the defined --scope were still installed from the npm registry. This commit fixes the code handling `--force-local` logic, so that: - Local packages are resolved as links within the monorepo - The following warning IS NOT printed anymore: _Installing local packages that do not match filters from registry_ Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@@ -319,9 +319,6 @@ Object { | |||
"bar@^2.0.0", | |||
"foo@^1.0.0", | |||
], | |||
"packages/package-4": Array [ | |||
"package-3@^1.0.0", | |||
], |
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.
Here!
This snapshot shows which dependencies were resolved from the npm registry.
Before my change, the dependency of packages/package-4
on package-3
was resolved from the npm registry.
After my change, package-3
is resolved as a link within monorepo. (See changes in the second snapshot below.)
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.
Can you write a (failing) test that illustrates the original problem, instead of changing the snapshot of an existing test?
@@ -161,7 +161,7 @@ class BootstrapCommand extends Command { | |||
chain = chain.then(filteredPackages => { | |||
this.filteredPackages = filteredPackages; | |||
|
|||
if (filteredPackages.length !== this.targetGraph.size) { | |||
if (filteredPackages.length !== this.targetGraph.size && !this.options.forceLocal) { |
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.
Theoretically, this is what the targetGraph
on line 168 was supposed to be doing:
this.targetGraph = new PackageGraph(filteredPackages, "allDependencies", this.options.forceLocal);
I'm not sure this is the appropriate change.
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.
Theoretically, this is what the targetGraph on line 168 was supposed to be doing:
Here is the problem: filteredPackages
contains only packages in scope (as determined via --scope
flag).
(EDITED for more clarity:) When I run the new test I have added in e0b0f99, the one running await lernaBootstrap(testDir)("--scope", "package-4", "--force-local");
:
- Original
targetGraph
contains entries for the following packages:package-3
,@test/package-1
,package-2
,package-4
filteredPackages
contains entries forpackage-4
only- When
new PackageGraph
is called, it creates entries forpackage-4
only. This looks reasonable to me, because we gave the constructor a list of packages containingpackage-4
only.
Another problem I see in the current implementation: even when I provide --force-local
, lerna bootstrap
prints the warning Installing local packages that do not match filters from registry. That does not make sense to me - as I understand --force-local
, it's purpose is to ensure that local packages are symlinked. So either --force-local
is not doing what it advertises, or the warning is lying.
I'm not sure this is the appropriate change.
I see.
Here is the problem I need to fix: when I run lerna bootstrap --scope package-4
, I want lerna to symlink all local packages (@test/package-1
, package-3
). This used to be the behavior before 71174e4 was landed and I want to get it back :)
Now my assumption is that --force-local
is supposed to address my needs but does not work as intended right now. If that's right and you still think my change is not appropriate, then could you please advise me how to proceed to get the issue fixed? It's enough to give me pointers to other places where to look.
If my assumption is not correct and the current behavior of --force-local
is intended, then could you please advise me what would be an acceptable solution/change in lerna that will give me:
- all local dependencies installed as symlinks, regardless of
--scope
arguments - no warnings printed to console
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.
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@evocateur thank you for a quick response ❤️
Sure. If we wanted to keep using snapshot-based assertions, then I could pretty much copy lerna/commands/bootstrap/__tests__/bootstrap-command.test.js Lines 307 to 314 in 959a00a
I don't see much value in that, therefore in e0b0f99 I am adding a new test with explicit assertions. This test fails with the following message:
|
@evocateur ping, have you have a chance to look at my response to your review comments? |
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.
Thanks for the deep explanations, I appreciate it. Sorry it took me so long to respond, life with a toddler leaves little time for open-source.
@evocateur thank you for accepting my contribution ❤️ I totally understand your situation, my kids were toddlers just few years ago 👶 I really appreciate the time you took away from your family to help me solve the issue we are experiencing in our project 🙇 |
Description
Before this change,
--force-local
option was not respected and monorepo-local packages outside of the defined --scope were still installed from the npm registry.This commit fixes the code handling
--force-local
logic, so that:Motivation and Context
In https://github.com/strongloop/loopback-next, we have a test that runs
lerna bootstrap --scope {pkg}
to bootstrap a single package. I noticed that our test starts to fail when the code in the package depends on changes made elsewhere in the monorepo and those changes were not released to the npm registry yet.After further digging I found that lerna is installing monorepo-local dependencies from the npm registry, instead of creating symlinks.
Eventually, I found 71174e4709 which changed the behavior of
lerna bootstrap
command in what I would consider a backwards-incompatible way, but fortunately added--force-local
flag.Except
--force-local
does not work right now!How Has This Been Tested?
It turns out the existing snapshot-based test for
--force-local
was using snapshot values describing the wrong behavior. I have updated these snapshots to match the desired outcome.Types of changes
Checklist:
@evocateur You are the author of
--force-local
option, could you please review my pull request? Thanks! 🙏