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

Fix pack includes files from node_modules, closes #2047 #2063

Merged

Conversation

torifat
Copy link
Member

@torifat torifat commented Nov 29, 2016

Summary

  • Only look for mandetory files (package.json, readme, license, etc) in root
  • Stop fs.walk inside ignored dirs (node_modules, .git, .svn, etc), makes pack way faster

Test plan

With only webpack dependency:

before

 ❯ yarn pack
Alias tip: y pack
yarn pack v0.17.8
warning package.json: No license field
success Wrote tarball to "/Users/rifat/Developments/test/yarn/yarn-test-v1.0.0.tgz".
✨  Done in 12.54s.

after

 ❯ yarn-dev pack
Alias tip: yd pack
yarn pack v0.18.0-0
warning package.json: No license field
success Wrote tarball to "/Users/rifat/Developments/test/yarn/yarn-test-v1.0.0.tgz".
✨  Done in 0.08s.

Fixes #1114, #2047

@torifat torifat force-pushed the fix/pack-includes-file-from-node-modules branch 6 times, most recently from 23f9935 to 4157c7a Compare November 29, 2016 04:31
@bestander
Copy link
Member

@torifat, any idea why the pack test failed on Travis but not on others?

@torifat
Copy link
Member Author

torifat commented Dec 4, 2016

@bestander Same as this one: https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/pack.js#L104

I will try to figure it out.

@torifat torifat force-pushed the fix/pack-includes-file-from-node-modules branch 4 times, most recently from 059db33 to 47a6282 Compare December 8, 2016 14:54
- Only look for mandetory files (`package.json`, `readme`, `license`, etc) in root
- Stop `fs.walk` inside ignored dirs (`node_modules`, `.git`, `.svn`, etc), makes `pack` way faster
@torifat torifat force-pushed the fix/pack-includes-file-from-node-modules branch from 47a6282 to c4eed59 Compare December 8, 2016 14:56
@torifat
Copy link
Member Author

torifat commented Dec 8, 2016

@bestander I have figured it out.

@torifat torifat requested a review from bestander December 8, 2016 15:19
@bestander bestander merged commit d1d5343 into yarnpkg:master Dec 8, 2016
@torifat torifat deleted the fix/pack-includes-file-from-node-modules branch December 8, 2016 16:28
@torifat torifat removed the request for review from bestander December 8, 2016 16:28
@SimenB
Copy link
Contributor

SimenB commented Dec 8, 2016

Sneaking in a thank you to @torifat for introducing me to https://github.com/djui/alias-tips, now I can finally learn more of oh-my-zsh's git aliases :D

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