Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

NPM packages don't have pinned versions. #2555

Open
1 task done
MicahZoltu opened this issue Nov 5, 2019 · 14 comments
Open
1 task done

NPM packages don't have pinned versions. #2555

MicahZoltu opened this issue Nov 5, 2019 · 14 comments

Comments

@MicahZoltu
Copy link
Contributor


Issue

npm install @truffle/workflow-compile will result in getting different transitive dependencies depending on what is available from NPM at install time. While this is a specific problem that can be addressed, the bigger issue is that none of the distributed NPM packages appear to include a package-lock.json which I believe is required by NPM in order to fetch fixed versions. Without it, dependency versions from package.json will be used and any transitive dependencies will follow whatever versioning strategy they have internally.

Steps to Reproduce

  1. Create new project.
  2. npm install @truffle/workflow-compile
  3. Commit (including package-lock.json) and push to GitHub.
  4. Wait for automated security check in the background.
  5. Receive email notifying you of security issues with packages.

Expected Behavior

All Truffle packages published to NPM include a package-lock.json file that includes a fixed set of dependencies (including transitives).

Actual Results

All Truffle packages appear to be published to NPM without a package-lock.json file, which means dependency versions are calculated an user install time.

Environment

  • Operating System: Windows 10
  • Ethereum client: N/A
  • Truffle version (truffle version): 5.0.43
  • node version (node --version): 12.3.0
  • npm version (npm --version): 6.9.0
@gnidan
Copy link
Contributor

gnidan commented Nov 6, 2019

We definitely want to look into this, perhaps urgently.

My biggest question is: what do other projects that use Lerna/Yarn do about this?

Also cc @davidmurdoch because I may recall your mentioning this in the past? Or was that something else?

@gnidan
Copy link
Contributor

gnidan commented Nov 6, 2019

Also @eggplantzzz makes a strong case that this is glaringly unbelievable and warrants reproduction.

@davidmurdoch
Copy link
Member

davidmurdoch commented Nov 6, 2019

Truffle uses yarn, which doesn't generate a package-lock.json or npm-shrinkwrap.json (note: only npm-shrinkwrap.json can be published to npm). If npm-shrinkwrap.json files are never published, npm will always respect semver for all dependencies and transitive dependencies based on the contents of the package.json.

Example:

If @truffle/fictional-package@1.1.1's package.json contains:

"dependencies": {
  "some-package": "1.2.3"
}

and some-package@1.2.3's package.json contains:

"dependencies": {
  "other-package": "^4.5.6"
}

then npm install @truffle/fictional-package@1.1.1 will create a dependency tree as follows:

`@truffle/fictional-package@1.1.1`
  | some-package@1.2.3
    | other-package@4.5.6

If at some point in the future other-package publishes v4.5.7 npm install @truffle/fictional-package@1.1.1 will now result a dependency tree like:

`@truffle/fictional-package@1.1.1`
  | some-package@1.2.3
    | other-package@4.5.7 // <- *** 4.5.7 ***

(note: I'm not sure if [transitive] dependency npm-shrinkwrap.json files are used in the absence of a root package's npm-shrinkwrap.json).

The truffle package itself doesn't have this problem, AFAIK, because the package is fully bundled and contains no install-time dependencies (someone please correct me if I'm wrong).

Yarn doesn't actually have a mechanism for pinning dependencies for published packages and this limitation is an intentional design decision: https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all/ (and is the main reason why Ganache doesn't use yarn)

@MicahZoltu
Copy link
Contributor Author

The truffle package itself doesn't have this problem, AFAIK, because the package is fully bundled and contains no install-time dependencies (someone please correct me if I'm wrong).

I believe it does, just to a more limited extent:

{
  "dependencies": {
    "app-module-path": "^2.2.0",
    "mocha": "5.2.0",
    "original-require": "1.0.1"
  },
}

@MicahZoltu
Copy link
Contributor Author

Also @eggplantzzz makes a strong case that this is glaringly unbelievable and warrants reproduction.

Where can one read this strong case?

@davidmurdoch
Copy link
Member

The truffle package itself doesn't have this problem, AFAIK, because the package is fully bundled and contains no install-time dependencies (someone please correct me if I'm wrong).

I believe it does, just to a more limited extent:

{
  "dependencies": {
    "app-module-path": "^2.2.0",
    "mocha": "5.2.0",
    "original-require": "1.0.1"
  },
}

Well, I stand corrected! :-D

original-require doesn't have any external dependencies, so that one is effectively pinned. app-module-path doesn't have external dependencies either, but still allows for updated versions due to the loose semver. Then there is the pinned mocha which itself includes 23 pinned dependencies; I haven't checked if its transitive-dependencies are all pinned and locked (but I doubt it).

@MicahZoltu
Copy link
Contributor Author

Mocha has 86 transitive dependencies! It is one of those packages that brings in the kitchen sink with it. 😢

@gnidan
Copy link
Contributor

gnidan commented Nov 7, 2019

Where can one read this strong case?

It was over Zoom, in our internal Wednesday weekly ticket processing meeting. I took notes on the occurrence of the "strong case" but was unable to capture a summary. I better leave it to @eggplantzzz to summarize (if he can remember why the case seemed strong before we had all this new information)

@gnidan
Copy link
Contributor

gnidan commented Nov 7, 2019

@davidmurdoch thank you for presenting a strong case for shrinkwrap guarantees.

I believe there are other significant factors in deciding whether the benefits of those guarantees outweigh other costs:

  • There's likely a significant operational cost to switch Truffle's development to NPM
  • We'd lose the benefits of Yarn workspaces, which helps ensure we have valid truffle builds

Theoretically, we could achieve the guarantees we get in truffle in all packages by bundling all @truffle/* packages the same way, but bundling has been shown to be non-ideal for other valid reasons.

Since this all seems non-ideal, I am interested in the work that @kumavis has been doing for MetaMask with https://github.com/LavaMoat/lavamoat-browserify, although I haven't done the research to see how it impacts the consideration for Truffle.

@eggplantzzz
Copy link
Contributor

Incidentally, it seems maybe yarn workspaces are not meant for what we are using it for.
“Workspaces are not meant to be published” and “Yarn will use a single lockfile rather than a different one for each project”
https://yarnpkg.com/lang/en/docs/workspaces/

@eggplantzzz
Copy link
Contributor

Also related yarnpkg/yarn#5428

@fainashalts
Copy link
Contributor

After some discussion we've concluded that we would like to be able to lock dependencies for each individual package. Putting this note here for whoever takes this on!

@dongmingh
Copy link
Contributor

Currently we are working on bundling all Truffle packages independently. This should lock all versions of dependencies.

@dongmingh
Copy link
Contributor

cc @benjamincburns, this can be closed when your PR is done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants