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

Remove unused dependency wrappy #27

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

rickschubert
Copy link
Contributor

The dependency wrappy is nowhere used throughout the entire project which is why I removed it.

The dependency "wrappy" is nowhere used throughout the entire project
which is why I removed it.
@rickschubert rickschubert requested a review from a team as a code owner July 22, 2018 15:57
@rickschubert
Copy link
Contributor Author

Do I also need to add the package-lock.json to the pull request? I was unsure.

@brodycj
Copy link

brodycj commented Jul 22, 2018

Do I also need to add the package-lock.json to the pull request?

I would say "yes" assuming there are any changes as a result of npm install since because package-lock.json was already committed.

@rickschubert
Copy link
Contributor Author

@brodybits I just double checked and even removed the node_modules/ folder and reinstalled: No changes to the package-lock.json are coming through. Do you think this is expected as the dependency was unused? I am not too familiar with the base of a package-lock file.

@brodycj
Copy link

brodycj commented Jul 22, 2018

@rickschubert I stand corrected then, just updated my answer.

I think the explanation is that the wrapper dependency remains in package-lock.json because it is needed to satisfy other dependencies and newer versions of npm keep all dependencies at top level whenever possible.

I would like to make an additional comment that I suspect wrappy should be kept in dependencies and added to bundleDependencies since it it committed in node_modules/wrappy. I think this is done to support non-traditional installs, with no recent version of npm available. Leaving it up to npm people to comment further.

@rickschubert
Copy link
Contributor Author

@brodybits Thank you for the comments! Let's see what other people say, otherwise this PR can be gladly scrapped :)

@zkat
Copy link
Contributor

zkat commented Jul 23, 2018

Allow me to clarify: wrappy was added to package.json in in order to flatten it. Until recently, npm used a special kind of installation style where top level dependencies all had their own node_modules, which reduced deduplication. This was done to prevent some upgrade-related issues from biting us because of a bug with the node.js installer itself (it didn't rimraf existing npm installations before extracting the new version).

This patch is also fine, because it gets rid of it, and it's normal that it wasn't removed from pkglock. It is, indeed, used by several dependencies:

➜ npm ls wrappy
[email protected] /Users/zkat/Documents/code/work/npm
├─┬ [email protected]
│ └── [email protected]  deduped
├─┬ [email protected]
│ └── [email protected]  deduped
├─┬ [email protected]
│ └── [email protected]  deduped
├─┬ [email protected]
│ └─┬ [email protected]
│   └─┬ [email protected]
│     ├─┬ [email protected]
│     │ └── [email protected]
│     └─┬ [email protected]
│       └── [email protected]  deduped
└── [email protected]

And finally, to answer your questions about bundleDependencies, @brodybits: npm includes all dependencies of bundled dependencies in the bundle list, so wrappy will still be included, and yes, it will continue to be committed. :)

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

This LGTM! 👍

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

Successfully merging this pull request may close these issues.

3 participants