-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
[Deps] remove object.assign #114
base: main
Are you sure you want to change the base?
Conversation
Node.js 4+ support Object.assign() natively: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
=======================================
Coverage 94.23% 94.23%
=======================================
Files 35 35
Lines 347 347
Branches 119 119
=======================================
Hits 327 327
Misses 20 20
Continue to review full report at Codecov.
|
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.
There are bugs with native Object.assign
property enumeration order in node 4.x, so until we drop node 4, we shouldn't drop this dependency (nor should anyone else)
I think it's time to drop Node 4 support to remove the object.assign dependency. |
What’s the reason to drop it? Removing a dep isn’t particularly valuable, and breaking changes are the most harmful thing any package can do. |
The reasons to remove dependencies are simple, well known and also apply to this proposed change:
These upsides are worth more than supporting Node 4 in my opinion. Do you know any higher up package depending on this package with Node 4 as a requirement? |
Installation size/time is a non-issue; more deps is better - that's not "fragmentation", that's "each thing does one thing well", as opposed to "bloat"; I maintain it, and it's much easier for me to maintain packages (i have 450+ of them) when there's many small ones; there is no attack surface since I maintain it; the package uses a native solution whenever it's compliant, so it's identically as fast as not using the dep. I can't know if anyone's using it on node 4, but if even one human being is, why would I want to make their life harder? |
Thanks for the work you do first of all. You claim to be against bloat and making people's lives more hard. To back up my believe here are a few arguments:
Thank you for reading and I hope you reconsider your decisions to bring the goals of your packages back in line with the goals of the broader JS developer community. Finally, one last question to which I would like to hear your answer: Will you ever drop support for Node.js 4 and what would have to happen for you to decide to do so? |
Marvin’s post is rife with inaccuracies, in stark contrast to the rest of the series which is excellent. nolyfill sacrifices robustness and makes your project more brittle. tsconfig-paths has backported features to v3, and so there's no longer any need to update to v4. I’m not sure i would ever drop it. I suppose if eslint forced a breaking change, at that time I’d reevaluate what we supported, but that wouldnt ever drive a breaking change, only be incidental along with one. |
Node.js 4+ support Object.assign() natively:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign