Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Jun 9, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
959a00a
fix(bootstrap): respect --force-local option
bajtos May 23, 2019
e0b0f99
fixup! add a failing test
bajtos May 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions commands/bootstrap/__tests__/__snapshots__/bootstrap-command.test.js.snap
View file
Open in desktop
Expand Up @@ -319,9 +319,6 @@ Object {
"bar@^2.0.0",
"foo@^1.0.0",
],
"packages/package-4": Array [
"package-3@^1.0.0",
],
Copy link
Contributor Author

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

}
`;

Expand All @@ -332,6 +329,21 @@ Array [
"dest": "packages/package-4/node_modules/@test/package-1",
"type": "junction",
},
Object {
"_src": "packages/package-3",
"dest": "packages/package-4/node_modules/package-3",
"type": "junction",
},
Object {
"_src": "packages/package-3/cli1.js",
"dest": "packages/package-4/node_modules/.bin/package3cli1",
"type": "exec",
},
Object {
"_src": "packages/package-3/cli2.js",
"dest": "packages/package-4/node_modules/.bin/package3cli2",
"type": "exec",
},
]
`;

Expand Down
2 changes: 1 addition & 1 deletion commands/bootstrap/index.js
View file
Open in desktop
Expand Up @@ -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) {
Copy link
Member

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.

Copy link
Contributor Author

@bajtos bajtos May 24, 2019 •
edited

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.logger.warn("bootstrap", "Installing local packages that do not match filters from registry");

// an explicit --scope, --ignore, or --since should only symlink the targeted packages, no others
Expand Down