jsx-ast-utils icon indicating copy to clipboard operation
jsx-ast-utils copied to clipboard

[Deps] remove object.assign

Open ludofischer opened this issue 4 years ago • 7 comments

Node.js 4+ support Object.assign() natively: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

ludofischer avatar Nov 11 '21 21:11 ludofischer

Codecov Report

Merging #114 (e9c3582) into master (b7fb11c) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #114   +/-   ##
=======================================
  Coverage   94.23%   94.23%           
=======================================
  Files          35       35           
  Lines         347      347           
  Branches      119      119           
=======================================
  Hits          327      327           
  Misses         20       20           
Impacted Files Coverage Δ
src/values/expressions/ObjectExpression.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b7fb11c...e9c3582. Read the comment docs.

codecov[bot] avatar Nov 11 '21 21:11 codecov[bot]

I think it's time to drop Node 4 support to remove the object.assign dependency. What's the reason for still supporting Node 4?

ulrichstark avatar Dec 25 '23 20:12 ulrichstark

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.

ljharb avatar Dec 25 '23 21:12 ljharb

The reasons to remove dependencies are simple, well known and also apply to this proposed change:

  • reduce installation size/time
  • reduce fragmentation of packages
  • easier maintainability
  • less attack surface
  • native solutions are probably faster in runtime performance than user solutions

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? Do you think anyone is using this package on Node 4?

ulrichstark avatar Dec 25 '23 22:12 ulrichstark

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?

ljharb avatar Dec 25 '23 23:12 ljharb

Thanks for the work you do first of all. Your packages are widely used by many popular packages maintained by you and others. This is my last attempt to align this specific package closer to the goals you claim.

You claim to be against bloat and making people's lives more hard. I believe that with your decisions on backwards compatibility you have achieved the opposite of what you intended with this package and your other packages.

To back up my believe here are a few arguments:

  • This thread by Marvin and the linked article highlighting the issue (https://twitter.com/marvinhagemeist/status/1704907071936958947)
  • The package nolyfill to "replace [your packages] with their super lightweight alternatives" (https://github.com/SukkaW/nolyfill)
  • Pull requests like we are currently in or https://github.com/import-js/eslint-plugin-import/pull/2654 for example in which people are trying to make the dependency graph simpler or the runtime performance better, invest their time in this change to propose, but get rejected by you citing backwards compatibility reasons.
  • I don't see any support for your decision in any of the above sources. I rather see people discussing with you trying to improve the situation.
  • The benefits mentioned in my last comment are much more helpful to people than they make their lives more hard because installation size/time absolutely does matter and there is an increased attack surface because more code means more places security relevant bugs could be in.
  • Node.js 4 has not had any new releases for almost six years now (https://nodejs.org/en/about/previous-releases).

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. I would also like to emphasize once again that you are highly respected by me and hopefully by everyone else in the community, and I thank you for your work so far!

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?

ulrichstark avatar Dec 26 '23 09:12 ulrichstark

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.

ljharb avatar Dec 26 '23 15:12 ljharb