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

[Deps] remove object.assign #114

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ludofischer
Copy link

@codecov
Copy link

codecov bot commented Nov 11, 2021

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.

Copy link
Member

@ljharb ljharb left a 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)

@ljharb ljharb marked this pull request as draft December 28, 2021 06:49
@ulrichstark
Copy link

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?

@ljharb
Copy link
Member

ljharb commented Dec 25, 2023

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.

@ulrichstark
Copy link

ulrichstark commented Dec 25, 2023

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?

@ljharb
Copy link
Member

ljharb commented Dec 25, 2023

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?

@ulrichstark
Copy link

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 Improve resolve performance with many files import-js/eslint-plugin-import#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?

@ljharb
Copy link
Member

ljharb commented Dec 26, 2023

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.

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

Successfully merging this pull request may close these issues.

3 participants