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.

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

[BUG] npx doesn't work when in child workspace #2826

Closed
felipecrs opened this issue Mar 4, 2021 · 22 comments
Closed

[BUG] npx doesn't work when in child workspace #2826

felipecrs opened this issue Mar 4, 2021 · 22 comments
Assignees
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@felipecrs
Copy link
Contributor

Current Behavior:

If I run npx <binary> of a binary installed by a child's package.json, it doesn't find the binary as npm v7 now install the binaries under the parent node_modules.

Expected Behavior:

This used to work, as there was no workspaces concept before and therefore all the projects were linking binaries to their node_modules.

Steps To Reproduce:

Should be simple.

Environment:

Suggestion

I think this should not be fixed by npx itself, but instead, npm should create symlinks under the children's node_modules/.bin at least for the packages referenced in the children's package.json.

@felipecrs felipecrs added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Mar 4, 2021
@ljharb
Copy link
Contributor

ljharb commented Mar 4, 2021

I don't think this is something that was ever truly reliable. If you're expecting to run a local binary, it should be a direct dep/devDep.

If anything, it's a bug that bins are created at all for transitive deps.

@felipecrs
Copy link
Contributor Author

felipecrs commented Mar 5, 2021

This can be replicated:

  1. Open my repository: https://gitpod.io/#https://github.com/felipecrs/megatar
    • It has a package.json in the root, and another in the website subfolder.
    • docusaurus is set as dependency in the website folder.
    • docusaurus is NOT set as dependency in the root project
    • website is in the list of workspace of the root project
    • Gitpod will run npm install automatically in the root project, but feel free to git clean -fdx and run npm install by yourself
  2. Run cd website; npx docusaurus --version
    • It will fail: docusaurus is not in website/node_modules/.bin but in node_modules/.bin (from the root folder).

@ljharb
Copy link
Contributor

ljharb commented Mar 5, 2021

Ah, in the context of workspaces, i see what you mean. The bins should surely not be hoisted to the parent, even if the module itself is.

@darcyclarke
Copy link
Contributor

Can replicate & we're discussing internally how to fix this bug

@darcyclarke darcyclarke removed the Needs Triage needs review for next steps label Apr 9, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 28 milestone Apr 9, 2021
@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 9, 2021

I have some ideas:

  1. Make npx look up for node_modules/.bin folder up to the root folder /
  2. Create a symbolic link of the root's bin folder: website/node_modules/.bin -> ../node_modules/.bin
  3. Create a symbolic link of the modules defined in the child workspace: website/node_modules/.bin/docusaurus -> ../node_modules/.bin/docusaurus (seems to be the best approach to me)
  4. Install the bins only to where they belong, instead of installing docusaurus on the node_modules, install to website/node_modules. Seems reasonable, but it would be too bad as I would lose the ability to run npx docusaurus from the root folder

@ljharb
Copy link
Contributor

ljharb commented Apr 9, 2021

The last option makes the most sense to me; if you wanted to run a bin from the root it'd be in the root's package.json.

@felipecrs
Copy link
Contributor Author

What if there are two child projects which has docusaurus as dependencies? Would be good if npm would be smart enough to download it once (if the version constraints match of course), instead of downloading it twice in each repo.

That's why I think the third option would be better than the fourth.

@ljharb
Copy link
Contributor

ljharb commented Apr 9, 2021

Hoisting the dependency itself would make sense - but not the bin.

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 9, 2021

In a monorepo, I would like to have semantic-release defined in the root project, so it would be available for all the child projects, even the binary because I would need to run npx semantic-release in all the child projects.

The only solution which would match this requirement would be 1 and 2. But now I thought in a new one which builds on 4:

  1. Install the bins only to the project they belong, instead of installing child's binaries on the root's node_modules/.bin, install to child's node_modules/.bin. Exceptionally for the bins defined in the root project, install the bins into root's node_modules/.bin and create symbolic links of all the binaries from the root project to all the child projects (unless they collide, of course).

Now, this is the one which makes more sense to me.

@ljharb
Copy link
Contributor

ljharb commented Apr 9, 2021

Yes, option 5 is what I'm expecting, with npm's current hoisting model.

@felipecrs
Copy link
Contributor Author

Apart from that, I think option 1 could be implemented as well, together with 5, and it would be harmless. No? It would only make npx smarter IMHO. As the require.resolve works.

@ljharb
Copy link
Contributor

ljharb commented Apr 9, 2021

Either or both could be done, yes, but workspace correctness would still require option 2, 3, 4, or 5.

@darcyclarke darcyclarke added the Priority 1 high priority issue label Apr 14, 2021
@ruyadorno
Copy link
Contributor

1. Make `npx` look up for `node_modules/.bin` folder up to the root folder `/`

That's the current behavior today: https://github.com/npm/run-script/blob/2a2f1b637ffa2463a82df477e17ea4e146682f5b/lib/set-path.js#L19


I'm not sure if there was something that was fixed between your original post and now but this is working as intended when I test with npm@7.10.0, I have the following structure (with @ruyadorno/create-index being a dep of my workspace a):

├── a
│  └── package.json
├── node_modules
│  ├── .bin
│  │  └── create-index -> ../@ruyadorno/create-index/create-index.js
│  ├── .package-lock.json
│  ├── @ruyadorno
│  │  └── create-index
│  │     ├── create-index.js
│  │     ├── package.json
│  │     └── README.md
│  └── a -> ../a
├── package-lock.json
└── package.json

it works as intended when I cd ./a && npm exec @ruyadorno/create-index

This is what both package.json files look like:

diff --git a/a/package.json b/a/package.json
new file mode 100644
index 0000000..c845857
--- a/a/package.json
+++ b/a/package.json
+{
+  "name": "a",
+  "version": "1.0.0",
+  "dependencies": {
+    "@ruyadorno/create-index": "^1.0.0"
+  }
+}
diff --git a/package.json b/package.json
new file mode 100644
index 0000000..23dd3e2
--- a/package.json
+++ b/package.json
+{
+  "name": "test-npx-workspaces",
+  "workspaces": [
+    "a"
+  ]
+}

I'll sync up internally with @darcyclarke again to see if he can still reproduce it but would appreciate if you can give it a try again @felipecrs, vlw!

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 22, 2021

The issue can still be replicated with the steps I suggested:

image


That's the current behavior today: https://github.com/npm/run-script/blob/2a2f1b637ffa2463a82df477e17ea4e146682f5b/lib/set-path.js#L19

If that would be the case, the example above would have worked. Perhaps such logic only applies to npm run and not npx?

@ruyadorno

This comment has been minimized.

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 26, 2021

@ruyadorno I'm sorry, but did you note that npx docusaurus in the root workspace works? And it's not only about docusaurus, it also happens with typedoc, which follows the rule you pointed:

> gitpod /workspace/megatar/website $ npx typedoc --version
Need to install the following packages:
  typedoc
Ok to proceed? (y) 
> gitpod /workspace/megatar/website $ cd ..
> gitpod /workspace/megatar $ npx typedoc --version
Info: Loaded plugin /workspace/megatar/node_modules/typedoc-plugin-markdown
Info: Loaded plugin /workspace/megatar/node_modules/typedoc-plugin-merge-modules

TypeDoc 0.20.32
Using TypeScript 4.2.4 from /workspace/megatar/node_modules/typescript/lib

So, the issue remains valid.

@ruyadorno
Copy link
Contributor

ruyadorno commented Apr 27, 2021

@felipecrs you're right! I totally got confused by the name of the packages there, now I realize the actual package that install the docusaurus name you're using is @docusaurus/core that is inside your website workspace 🤦 I'm really sorry for the confusion.

I believe I'm on the right track now and I think this issue should be solved by looking up as you suggested in your #1 option in libnpmexec since that's the behavior of @npmcli/run-script which is used to actually run these (the final solution might differ but I'll open a PR to discuss it with the rest of the team). Thanks again!

@ruyadorno ruyadorno reopened this Apr 27, 2021
@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

@ruyadorno #2826 (comment)

@ruyadorno
Copy link
Contributor

@ruyadorno #2826 (comment)

@ljharb I'm carefully avoiding that topic here since I believe it's going to be better addressed as part of the upcoming workspaces-revamp RFC we have been alluding to in the last OpenRFC meetings 😉

That said, I'm not excluding the possibility the solution to the problem surfaced by this issue might have to pass through that major overhaul. For now I'm exploring the possibility of fixing this by simply reusing the lookup algo from run-script (which btw might not be so simple) either way I'll keep you posted 😊 Thanks!

@ljharb
Copy link
Contributor

ljharb commented Apr 27, 2021

Thanks, as long as it's not ignored :-)

ruyadorno added a commit to ruyadorno/libnpmexec that referenced this issue Apr 27, 2021
Currently @npmcli/run-script already supports this walk up lookup logic
that allows any nested folder to use a bin that is located inside a
node_modules/.bin folder higher in the directory tree.

This commit adds the same logic from:
https://github.com/npm/run-script/blob/47a4d539fb07220e7215cc0e482683b76407ef9b/lib/set-path.js#L24-L28
to libnpmexec so that users may use a binary from a dependency of a
nested workspace in the context of that workspaces' folder.

Fixes: npm/cli#2826
@ruyadorno
Copy link
Contributor

ruyadorno commented Apr 28, 2021

@felipecrs I also forgot to mention that alternatively you can also use npm exec workspaces support (I don't remember exactly when we shipped this feature, might have been after you originally opened the issue), from the root level:

$ npm exec -w website -- docusaurus

so you can also use that while you wait for the fix 😊

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 28, 2021

Thank you @ruyadorno. Very nice feature.

ruyadorno added a commit to ruyadorno/libnpmexec that referenced this issue Apr 29, 2021
Currently @npmcli/run-script already supports this walk up lookup logic
that allows any nested folder to use a bin that is located inside a
node_modules/.bin folder higher in the directory tree.

This commit adds the same logic from:
https://github.com/npm/run-script/blob/47a4d539fb07220e7215cc0e482683b76407ef9b/lib/set-path.js#L24-L28
to libnpmexec so that users may use a binary from a dependency of a
nested workspace in the context of that workspaces' folder.

Fixes: npm/cli#2826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants