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

docs: add pnpm #11388

Merged
merged 4 commits into from Sep 11, 2018
Merged

docs: add pnpm #11388

merged 4 commits into from Sep 11, 2018

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented Jul 7, 2018

Description

Adds pnpm install command. pnpm is "performant npm".

Motivation and Context

Screenshots (if appropriate):

Types of changes

  • Documentation
  • 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 (all required):

  • I have read and follow the CONTRIBUTING document.
  • There are no other pull request similar to this one.
  • The pull request title is descriptive.
  • The template is fully and correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).
  • All new and existing tests passed.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

https://pnpm.js.org/docs/en/limitations.html

npm-shrinkwrap.json and package-lock.json are ignored. Unlike pnpm, npm can install the same name@version multiple times and with different sets of dependencies. npm's shrinkwrap file is designed to reflect the node_modules layout created by npm. pnpm cannot create a similar layout, so it cannot respect npm's lockfile format.

This is a serious issue. Anyone collaborating on a project with pnpm would ignore lockfiles, install different dependencies, may discover bugs specific to its even environment and introduce bugs to the project because of that.

As every npm package is already installable by pnpm (like yarn), adding the pnpm installation command would only be promotion and I don't think we should promote it because of its limitations.

What do you think ?

@DanielRuf
Copy link
Contributor Author

In general pnpm takes a totally different approach than npm and yarn (symlinked dependencies) and does many things better.

@zkochan how do you see this?

@zkochan
Copy link

zkochan commented Jul 7, 2018

adding the pnpm installation command would only be promotion and I don't think we should promote it because of its limitations.

I maintain pnpm for 2 years and there was not a single issue caused by npm-shrinkwrap.json. I understand that it could happen but it could happen also with Yarn.

If you want everyone to have the same environment, you should probably suggest doing the installation with a specific version of a specific package manager.

There is a bigger chance to catch an issue with pnpm then to introduce bugs. pnpm's strict (non-flat) node_modules doesn't allow code to require packages that are not declared in package.json.

If you prefer to not include pnpm because you feel like it is not popular enough. That's OK. But I don't want anyone to think/claim that pnpm is not reliable. It is heavily used by some big clients for months already

@ncoden
Copy link
Contributor

ncoden commented Jul 8, 2018

Hi @zkochan

We do not have the same issue with Yarn as it is now quite popular and provide a yarn.lock lockfile that is used in a lot of popular project. It is simple to keep it up-to-date alongside package-lock.json and some GitHub dependency managers like dependabot handle it well.

I took at better look at your documentation and seen that you have your own lockfile shrinkwrap.yaml. I think we can add pnpm installation command to the documentation, but not before adding a shrinkwrap.yaml lockfile compatible with others lockfiles, like we do for Yarn.

However, I don't find any way to do that. Unlike yarn import and npm i --package-lock-only that generate a lockfile from others lockfiles or installed node_modules, pnpm i --shrinkwrap-only will link node_modules then generate shrinkwrap.yaml in such a way that modules are always updated to the latest versions.

Doing the installation with pnpm first then generating the npm/yarn lockfiles from it is not a reliable solution neither as our contributors may not want to use pnpm and rebuild their node_modules folder. Also because the linked node_modules, npm and yarn considers all installed package dependencies as pinned (1.0.0 instead of ^1.0.0). I would not be able to say exactly why, but I guess it comes from the way dependencies are organized in the pnpm store.

Would you know how we could continue to support and update both npm, yarn and pnpm lockfiles ?
Something like pnpm import similar to yarn import would be useful, like making pnpm i --shrinkwrap-only working with non-linked node_modules.

What do you think ?

@zkochan zkochan mentioned this pull request Jul 8, 2018
@zkochan
Copy link

zkochan commented Jul 8, 2018

I created an issue in the pnpm repo to implement pnpm import.

ref pnpm/pnpm#1266

@zkochan
Copy link

zkochan commented Sep 11, 2018

pnpm now has a pnpm import command that can convert a package-lock.json/npm-shrinkwrap.json to shrinkwrap.yaml

@ncoden ncoden self-assigned this Sep 11, 2018
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thank you @DanielRuf and @zkochan!

@ncoden ncoden merged commit bf22a83 into foundation:develop Sep 11, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Sep 11, 2018
….5.0

3d83335 docs: add pnpm
3fae21d chore: add pnpm lockfile
bfaecd2 chore: add npm script to generate all lockfiles

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@ncoden ncoden mentioned this pull request Sep 11, 2018
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants