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] npm ls executed within workspace does not look up node_modules in workspace #3378

Closed
1 task done
panayot-cankov opened this issue Jun 7, 2021 · 6 comments
Closed
1 task done
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release

Comments

@panayot-cankov
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Using vsce in vscode extension workspace throws errors for missing dependencies.

I have several VSCode extensions that am moving together in monorepo with npm workspace.
All the dependencies for the extensions are hoisted at the root node_modules.
To prepare the vscode extension packages I use the vsce tool, that internally uses:

npm list --production --depth=99999 --loglevel=error

to collect all npm modules that need to be backed in the VSCode extension.
The command is executed in the directory of each workspace, and fails with:

npm ERR! missing: @aspnet/signalr@^1.1.4, required by [email protected]
npm ERR! missing: @types/app-root-path@^1.2.4, required by [email protected]
npm ERR! missing: @types/archiver@^2.1.3, required by [email protected]

however these are already installed in node_modules a few folders up.

Expected Behavior

Using vsce in vscode extension workspace to pack correctly.

From what I understand it may need changes in that external vsce tool as well, but first the npm ls --production will need to either change behaviour to look in node_modules outside the current folder, or perhaps a new switch like --include-workspace-deps to look modules up. I hope I am missing something that could get this running.

Steps To Reproduce

Create a project with few workspaces.
Install the dependencies in at the root.
cd into one of the workspaces.
run npm list

It will generate errors for missing dependencies, that had been installed at the root level.

I've prepared a small repo that shows the list failing if the deps are installed at the root level, and how it works when the deps are installed locally in each dependency:

https://github.com/panayot-cankov/npm-list-repro

Environment

  • OS: Windows
  • Node: 14.16.0
  • npm: 1.16.0
@panayot-cankov panayot-cankov 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 Jun 7, 2021
@ljharb
Copy link
Contributor

ljharb commented Jun 7, 2021

What happens if the npm ls command omits depth entirely?

@panayot-cankov
Copy link
Author

@ljharb I am not sure what do you mean by omit depth entirely, but in the attached repo github.com/panayot-cankov/npm-list-repro it errors with and without the --depth switch.

@ruyadorno
Copy link
Contributor

@panayot-cankov some points to note:

  • In npm7 instead of using --depth=99999 you may use --all instead
  • The ability to navigate into a folder and run npm commands from there is not a supported workflow as of today, the alternative is to execute commands from the root folder specifying the workspace name OR path, e.g: npm ls -w workspace-a - in case you're interested in that specific workflow (cd <path> + npm cmd) it's currently subject of a RFC proposal, you can read more about it here: RFC: npm workspaces: auto switch context based on cwd rfcs#343
  • Omitting dev dependencies for workspaces in npm list is currently broken, I'll try to get a fix for that soon and I'm tracking it here: [BUG] npm ls --prod is not omitting dev deps of workspaces #3382

I believe this issue can be closed since any further conversation about wider support to top-level npm cli commands inside workspaces folders can continue in the RFC (but happy to reopen it if that's not the case) 😊

Thanks for the thorough report and examples!

@pana-cc
Copy link

pana-cc commented Jun 8, 2021

I will need this reopen.

While a lot of the npm scripts now can run at the root level and have ‘--workspaces’ or ‘--workspace=‘, these are still used to trigger scripts in each individual workspace. For example ‘npm run build --workspaces’ is supposed to execute the build script in each workspace. However the actual packaging and build scripts are not moved into the root level, and the ‘npm list’ in this scenario is part of the build process of each individual workspace. It makes perfect sense to me to have a command that semantically does:

Get my package and list all production dependencies recursively, try to minimise duplicates where possible and then map them to file locations on my file system where they are currently installed.

I can understand how this may be a complex task, but I don’t understand why it can’t list path to mocha like ‘../../node_modules/mocha/‘ when the command is run and the modules are located somewhere up the file system tree.

This is not the first time I see ‘npm list’ used like that. Several packaging mechanism use ‘npm list’ and probably they will have to move away from it. For one I worked on the NativeScript framework and it had similar packaging implementation before it moved to webpack packaging.

And finally, this is not a workspace issue, node was able to require node_modules up the directory tree outside the current package before npm introduced workspaces.

And the workspaces are awesome, I have 3 VSCode extensions each has a web client and an extension package, mocha used to install 6 times in each package, now it installs once. I just hope you will look into it so we can build on top of a stronger npm rather than having to split in npm vs yarn (which btw he issues with workspaces as well) or author ‘a-true-npm-list’ that does 1 thing right and 10 wrong...

@panayot-cankov
Copy link
Author

On the other hand that part of the RFC:

Make a recursive check that walks up the file system structure and validates if the current prefix is defined as the "workspaces" property of a package.json file of a parent directory.

In case that is so, we can then tweak the internal contexts/variables in such a way that runs the command in the context of the top-level workspace while defining the current working directory as the workspace configuration to be used.

makes perfect sense, and if the npm ls --workspace=myvscodeextension already works (with fixed --prod) it will require zero additional configurations, and will also be backward compatible, as in old repos the npm ls won't look up in the package's parent directories.

@ruyadorno
Copy link
Contributor

ruyadorno commented Jun 11, 2021

@pana-cc thanks, I still believe it's better to focus our efforts around npm/rfcs#343 in order to figure out a better workflow that can suit all npm commands instead of trying to track each individual command in many issues across the npm cli repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

4 participants