fix(bootstrap): respect --force-local option #2104

bajtos · 2019-05-23T10:48:49Z

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:

Local packages are resolved as links within the monorepo, as one would expect.
The following warning IS NOT printed anymore: Installing local packages that do not match filters from registry

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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

@evocateur You are the author of --force-local option, could you please review my pull request? Thanks! 🙏

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>

bajtos · 2019-05-23T10:50:24Z

commands/bootstrap/__tests__/__snapshots__/bootstrap-command.test.js.snap

@@ -319,9 +319,6 @@ Object {
    "bar@^2.0.0",
    "foo@^1.0.0",
  ],
-  "packages/package-4": Array [
-    "package-3@^1.0.0",
-  ],


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.)

evocateur

Can you write a (failing) test that illustrates the original problem, instead of changing the snapshot of an existing test?

evocateur · 2019-05-23T17:23:45Z

commands/bootstrap/index.js

@@ -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) {


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.

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 for package-4 only

When new PackageGraph is called, it creates entries for package-4 only. This looks reasonable to me, because we gave the constructor a list of packages containing package-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

Conceptually:

The block if (filteredPackages.length !== this.targetGraph.size) { /* use scoped targetGraph */ } was introduced by 71174e4.

In my change, I am skipping this block entirely when options.forceLocal is enabled, thus reverting back to behavior before 71174e4.

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos · 2019-05-24T07:01:36Z

@evocateur thank you for a quick response ❤️

Can you write a (failing) test that illustrates the original problem, instead of changing the snapshot of an existing test?

Sure. If we wanted to keep using snapshot-based assertions, then I could pretty much copy should respect --force-local test as-is.

lerna/commands/bootstrap/__tests__/bootstrap-command.test.js

Lines 307 to 314 in 959a00a

    
           it("should respect --force-local", async () => { 
        
             const testDir = await initFixture("basic"); 
        
             await lernaBootstrap(testDir)("--scope", "@test/package-1", "--scope", "package-4", "--force-local"); 
        
             expect(installedPackagesInDirectories(testDir)).toMatchSnapshot(); 
        
             expect(symlinkedDirectories(testDir)).toMatchSnapshot(); 
        
           });

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:

FAIL  commands/bootstrap/__tests__/bootstrap-command.test.js (5.061s)
  ● BootstrapCommand › with local package dependencies › should respect --force-local when a single package is in scope

    expect(received).toEqual(expected) // deep equality

    - Expected
    + Received

    - Array []
    + Array [
    +   "@test/package-1@^0.0.0",
    +   "package-3@^1.0.0",
    + ]

bajtos · 2019-05-30T07:25:21Z

@evocateur ping, have you have a chance to look at my response to your review comments?

evocateur

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.

bajtos · 2019-06-10T06:08:10Z

@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 🙇

bajtos commented May 23, 2019

View reviewed changes

This was referenced May 23, 2019

test(cli): fix lerna bootstrap to link monorepo packages loopbackio/loopback-next#2944

Merged

feat(build): enable incremental compilation loopbackio/loopback-next#2928

Merged

evocateur suggested changes May 23, 2019

View reviewed changes

fixup! add a failing test

e0b0f99

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos mentioned this pull request May 24, 2019

lerna add --scope does not update package-lock file #1989

Closed

evocateur approved these changes Jun 9, 2019

View reviewed changes

evocateur merged commit c2fb639 into lerna:master Jun 9, 2019

bajtos deleted the fix/bootstrap-force-local branch June 10, 2019 06:05

bajtos mentioned this pull request Jun 10, 2019

Update lerna to 3.15.0 loopbackio/loopback-next#3075

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bootstrap): respect --force-local option #2104

fix(bootstrap): respect --force-local option #2104

bajtos commented May 23, 2019 •

edited

bajtos May 23, 2019

evocateur left a comment

evocateur May 23, 2019

bajtos May 24, 2019 •

edited

bajtos May 24, 2019

bajtos commented May 24, 2019 •

edited

bajtos commented May 30, 2019

evocateur left a comment

bajtos commented Jun 10, 2019

fix(bootstrap): respect --force-local option #2104

fix(bootstrap): respect --force-local option #2104

Conversation

bajtos commented May 23, 2019 • edited

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

bajtos May 23, 2019

Choose a reason for hiding this comment

evocateur left a comment

Choose a reason for hiding this comment

evocateur May 23, 2019

Choose a reason for hiding this comment

bajtos May 24, 2019 • edited

Choose a reason for hiding this comment

bajtos May 24, 2019

Choose a reason for hiding this comment

bajtos commented May 24, 2019 • edited

bajtos commented May 30, 2019

evocateur left a comment

Choose a reason for hiding this comment

bajtos commented Jun 10, 2019

bajtos commented May 23, 2019 •

edited

bajtos May 24, 2019 •

edited

bajtos commented May 24, 2019 •

edited