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

Use pnpm instead of npm for frontend resources #7021

Closed
mehdi-vaadin opened this issue Nov 25, 2019 · 3 comments · Fixed by #7059
Closed

Use pnpm instead of npm for frontend resources #7021

mehdi-vaadin opened this issue Nov 25, 2019 · 3 comments · Fixed by #7059

Comments

@mehdi-vaadin
Copy link
Contributor

mehdi-vaadin commented Nov 25, 2019

Part of #6966
Depends on #7019

With setting a configuration key, e.g. disable.pnpm (the same key is used in #7020), the user should be able to switch to npm.
The new method that is created as part of #7019 in FrontendUtils should be used to get the pnpm executable command.

@mehdi-vaadin mehdi-vaadin changed the title Use globally installed pnpm instead of npm for frontend resources Use pnpm instead of npm for frontend resources Nov 27, 2019
@joheriks
Copy link
Contributor

joheriks commented Nov 27, 2019

Acceptance criteria:

  • pnpm (local or global) is used in all cases where npm is currently used.
  • there is an IT with disabled pnpm to ensure that npm-only mode still works

@denis-anisimov
Copy link
Contributor

denis-anisimov commented Dec 3, 2019

I have a lot of issues with this task.
I hope I will be able to solve them one by one but I would like to write them down here for the reference:

  • pnpm installs packages in a specific way. It doesn't use flat structure for node_modules as npm. It installs the declared dependencies into node_modules but all transitive dependencies are installed into node_modules/.pnpm/registry.npmjs.org/. As a result node_modules contains only small set of declared dependencies and all other transitive deps are installed in a different folder. This is correct structure which should be handled properly. BUT webpack is not able to handle this structure at all. When you run it it fails with error because it tries to find a transitive dependency (e.g. mkdirp ) and it's not able to find it.
  • to be able to solve this there is a config parameter shamefully-hoist=true which says pnpm to install deps into node_modules . The parameter works being added into .nmprc (another question : is there a way to avoid using this file?). But when I run webpack I still get an error like this Error: Can't resolve '@vaadin/flow-frontend/dndConnector-es6.js?babel-target=es5' in '/Users/denis/workspace/flow/flow-tests/test-mixed/target/frontend'. Will see what can I do about it.
  • pnpm doesn't requires and doesn't make package-lock.json. But this file is created when pnpm is used instead of npm. It looks like our infrastructure made for npm is creating this file and we should do something about it. pnpm creates pnpm-lock.yaml. It might be that this file should be handled somehow in case of pnpm (may be in the code related to shrink-wrap version).

@denis-anisimov
Copy link
Contributor

shamefully-hoist=true can be specified via the command line via --shamefully-hoist=true.
That solves an issue with webpack . Also after complete cleanup test-mixed module started to work.

package-lock.json is created by npm when pnpm is installed locally (not globally). So this is fine: we should not care about it.

There is an issue related to target/frontend/package.json : deps from it are not read and installed.
That will be solved upcoming PR which merges this file into main package.json.

One more thing that remains and we should care about it separately ( I will make a ticket about it) :
shrink wrap version check .
It's not clear whether we need to check it at all with pnpm . One of the files that checked for shrink wrap version is package-lock.json. Since this file is not used by pnpm we should update logic accordingly.

@denis-anisimov denis-anisimov moved this from In progress to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Dec 4, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging a pull request may close this issue.

3 participants