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.

Sign up for GitHub

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

Jump to bottom

BREAKING CHANGE: Drop peer deps #40

Merged
merged 1 commit into from Oct 2, 2018
Merged

BREAKING CHANGE: Drop peer deps #40

merged 1 commit into from Oct 2, 2018

Conversation

lili2311
Copy link
Contributor

@lili2311 lili2311 commented Sep 25, 2018 •
edited by michael-go

With this breaking change the resolved tree will no longer resolve peerDependencies

Why?

  • Peer dependencies are currently incorrectly handled by this module, latest NPM forces users to manually install those so user would add them to package.json if they do decide to add them anyway.
  • The tree currently aims to represent "install relations" between dependencies rather than "use relations" - so if e.g. a project depends on both react and react-dropzone. The tree before this PR would duplicate the whole react sub-tree under react-dropzone although react wasn't installed by NPM due to the peerDependency on react in react-dropzone. So after this PR - react-dropzone will no longer have react as a dependency.
  • In many cases (such as projects using react.native e.g.) this will create drastically smaller trees

What will this PR do

Peer deps will be DROPPED, this will bring this packages dependency support more in line with https://github.com/snyk/nodejs-lockfile-parser. Also npm is no longer installing peer deps since npm >= 3 https://docs.npmjs.com/files/package.json#peerdependencies

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2018 •
edited

CLA assistant check
All committers have signed the CLA.

@adrukh
Copy link
Contributor

adrukh commented Sep 25, 2018

O M G

@lili2311 lili2311 changed the title fix: wip Drop unused properties of peer & bundles as they add massively to tree size Sep 25, 2018
@michael-go
Copy link
Contributor

I think this should be a BREAKING CHANGE

@lili2311 lili2311 changed the title Drop unused properties of peer & bundles as they add massively to tree size Drop unused properties of peer & bundled as they add massively to tree size Sep 25, 2018
@lili2311 lili2311 changed the title Drop unused properties of peer & bundled as they add massively to tree size BREAKING CHANGE: Drop unused properties of peer & bundled as they add massively to tree size Sep 25, 2018
@michael-go
Copy link
Contributor

michael-go commented Sep 25, 2018 •
edited

  1. so does this PR mean we no longer collect peer & bundled dependencies ?

  2. suggest a better explanation is needed to why we decided to do this. I guess you mean the bundled and depType are "unused" by https://github.com/snyk/snyk , but this library might be used by other projects. And I think that the fact the dependencies exist in the tree is actually used today also by the Snyk CLI

@lili2311
Copy link
Contributor Author

@michael-go Added a better description & why, are you thinking to keep these deps still?

@michael-go
Copy link
Contributor

michael-go commented Sep 26, 2018 •
edited

can you please also mention that this not only removes annotations, but actual subtrees

@michael-go
Copy link
Contributor

  • regarding peerDependencies: I think it's fine we drop these - as the information loss is only partial, as the deps will usually still be deps of the root, just not duplicated again and again like today for every peerDependency occurrence.

  • regrading bundledDependencies: I agree the bundled array that exists today is strange, wasteful and not useful. But what about actually including bundledDependencies in the tree? what was happening before and will it change? Because bundled dependencies are installed by npm and we better not miss these when we build the tree.

@darscan can please take a look too?

@lili2311 lili2311 changed the title BREAKING CHANGE: Drop unused properties of peer & bundled as they add massively to tree size BREAKING CHANGE: Drop properties of peer & bundled as they add massively to tree size Sep 28, 2018
@lili2311
Copy link
Contributor Author

Bundled is being used in CLI, will re-check exactly for what and if it changes this PR

@lili2311 lili2311 force-pushed the fix/remove-unused-properties branch from 3daf9f8 to cfccfb7 Compare October 1, 2018 21:16
@lili2311 lili2311 changed the title BREAKING CHANGE: Drop properties of peer & bundled as they add massively to tree size BREAKING CHANGE: Drop peer deps Oct 1, 2018
@lili2311
Copy link
Contributor Author

lili2311 commented Oct 1, 2018

Left bundled deps alone for now as they are being used in CLI, will come back to fix those in a separate PR

@lili2311 lili2311 force-pushed the fix/remove-unused-properties branch from cfccfb7 to 53ca05b Compare October 1, 2018 21:31
@lili2311 lili2311 closed this Oct 2, 2018
@lili2311 lili2311 reopened this Oct 2, 2018
@lili2311 lili2311 closed this Oct 2, 2018
@lili2311 lili2311 reopened this Oct 2, 2018
michael-go
michael-go reviewed Oct 2, 2018
View reviewed changes
lib/dep-types.js Outdated
type = depTypes.PEER;
from = pkg.peerDependencies[depName];
}
var bundled = !!(pkg.bundleDependencies &&
Copy link
Contributor

@michael-go michael-go Oct 2, 2018 •
edited

Choose a reason for hiding this comment

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

any reason this was moved above the if (pkg.dependencies .. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None I can think of, let me move it back

@lili2311 lili2311 force-pushed the fix/remove-unused-properties branch from 53ca05b to 252904e Compare October 2, 2018 15:05
@lili2311 lili2311 merged commit b67e4d1 into master Oct 2, 2018
@lili2311 lili2311 deleted the fix/remove-unused-properties branch October 2, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-go michael-go michael-go approved these changes

Assignees

@lili2311 lili2311

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@lili2311 @CLAassistant @adrukh @michael-go