-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
update major versions for npm deps + remove log
option
#4427
Conversation
4d88ba9
to
81da28b
Compare
found 20 benchmarks with statistically significant performance improvements
timing results
|
8b3bcf6
to
b1cc530
Compare
log
option
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: the log option is no longer passed to the updated deps
BREAKING CHANGE: this drops support for the `log` property and the banner is shown using the silent option
BREAKING CHANGE: this drops support for the `log` property
BREAKING CHANGE: this drops support for the `log` property
BREAKING CHANGE: this removes the log option
cd72162
to
1054b10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is a doozy lol
we looked at this together during our pairing session, i then checked out this branch and kicked the tires briefly and saw no issues. i think this is good to land!
@lukekarrys I don't want to open a new issue, but happy to do it if you think it's worth it. There are still some duplicate packages that in theory could be deduplicated by updating the deps where needed upstream:
Should save a few bytes/files if these are deduped :) EDIT: these deps might be unused or used as devDependencies (needs a closer look):
|
It seems there are plenty of useless files still included too (untested): Files that could possibly be removed
BTW, nice to see that some work is done already in regards to the file size/number of files! https://packagephobia.com/result?p=npm%406.14.16%2Cnpm%407.24.2%2Cnpm%408.5.1 |
Most of those are because we're waiting on node-gyp to land its next semver major nodejs/node-gyp#2603 |
Both issues? BTW when I do |
This is interesting, but I'd be concerned with removing files from any valid dependencies due to the nature of JavaScript, dynamic requires, etc. Not saying we would never do that, but it seems like not the lowest hanging fruit vs the effort/impact.
I just checked both of those and they are deduped in production deps but not dev/docs. We're not as concerned about those. |
@XhmikosR That shouldn't be happening 😄 I just tested on my machine with a brand new clone of the repo and |
Thanks for the replies!
AFAICT you are already ignoring plenty of dev-related files from production deps hence why I suggested this. I'm not saying these are super important, I agree with you. I'm just saying it should be looked at later, because the gains should be significant :)
I suspect it's something related to Windows and symlinks, so as long as you get a clean repo that's enough for me :) |
I think it's a good suggestion, but I'm not aware of anywhere we are already doing this. We try to do it in all our own modules via |
#3362 and https://github.com/npm/cli/blob/latest/node_modules/.gitignore I do open upstream PRs when possible and I agree that is the optimal solution, but until then you could also leverage the current solution :) |
Thank you for showing me that we have a current solution already. I must've missed that we are doing that! 😄 |
This PR makes breaking changes to the following workspaces (all except
libnpmfund
):arborist
should be published first, and then bumped insidelibnpmexec
andlibnpmfund
before those are published.