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

remove object.entries dependency #2878

Merged
merged 4 commits into from
Jan 28, 2024
Merged

remove object.entries dependency #2878

merged 4 commits into from
Jan 28, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jan 4, 2024

This removes the object.entries package and uses the widely available built-in native Object.entries.

Many years ago, you made these changes:

"engines": {
"node": "^10.12.0 || >=12.0.0"
},

"engines": {
"node": "^10.12.0 || ^12.22.0 || ^14.17.0 || >=16.0.0"
}

Object.entries has been available since node 7.x according to MDN.

This means the dependency is now entirely redundant it seems, unless we want to revert the various changes that lead us to eslint >=7 support being enforced.

@ljharb
Copy link
Collaborator

ljharb commented Jan 6, 2024

This does happen to be one of the few builtin methods that shipped without bugs. What's the benefit of removing the dep tho? It'll already just use the native function, but in a way that's robust against other plugins mutating globals.

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

This does happen to be one of the few builtin methods that shipped without bugs. What's the benefit of removing the dep tho? It'll already just use the native function, but in a way that's robust against other plugins mutating globals.

it is redundant code, and an extra dependency consumers have to pull down for no good reason anymore. i'm pretty sure you understand that, there's no reason either of us can come up with to keep it here thanks to changing the engine range.

if you have an answer to the reverse question: what's the benefit of keeping it around? i'd love to hear

let me know if you want any other changes, and if you can trigger CI, that'd be much appreciated

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

The benefit of keeping it around is that delete Object.entries won't break it.

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

do you want to introduce patches for the following then too:

  • Array.isArray
  • Array.prototype.concat
  • Array.prototype.indexOf
  • Array.prototype.forEach

we can also delete all of those in an equally far fetched edge case.

it is a "none of them or all of them" situation. so if we're not going to do that, the benefit you mentioned isn't so relevant and we can probably move ahead with this.

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

Yes, in general that's what I'd want to do, especially because eslint's flat config requires a JS-based config, so eslint plugins can no longer rely on an unmodified environment like they used to.

It's definitely not all or none; robustness is a spectrum.

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

to summarise and make sure i understood correctly:

I want to remove this dependency because the engine constraint means it is redundant, it will always fall through to the underlying native implementation which all supported platforms now have.

you don't want to remove it because you want to account for the fact consumers can mutate the global namespace (not only Object, but all globals outside the scope of this change).

is this correct?

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

I haven't said I definitely won't remove it, to be clear, i'm just explaining why it has value even in modern engines. Otherwise yes, that's correct.

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

(it's also worth noting that since the eslint plugins this config requires support nodes that lack Object.entries, everyone using this config will have this dep installed regardless, so removing it isn't saving anyone any bandwith or disk space)

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

yup i'm aware the dependency will still be in the tree somewhere, that's fine.

one less redundant direct dependency is still a good thing.

on a side note, it does seem that your belief of a 'stable' environment is a topic for outside of this package (not necessarily instead of, but as well as). what you're doing here is trying to ensure it is impossible to get anything other than the underlying native Object.entries (and the same for all other globals). The platform evidently wasn't built that way (Object.entries can be removed, like everything else near enough), so maybe its a fundamental way the platform works that you disagree with rather than something specific to this package?

on the other side of that debate, there will be consumers that purposely mutate globals i'm sure. preventing them from doing so could lead to confusion.

the size of the group doing that, and the one breaking this config by changing globals, will be small though i'm sure.

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

It is definitely a fundamental way the platform works, which is why such defensive coding techniques are required.

Purposely mutating globals must be done before evaluating any other code, and if someone does that in a spec-compliant way, it won't cause any problems - and if it's not spec compliant, then those problems are impossible to avoid, and thus, nobody needs to consider that use case.

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

the CI failure happens in master too FYI

not entirely sure why that is, and don't fully understand what is considered an unused rule

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

You dont have to worry about those failures; I'll fix that myself before landing any PRs.

@kibertoad
Copy link

kibertoad commented Jan 8, 2024

@ljharb Have you ever in your career observed anyone using "delete Object.entries"? Or deleting any global Object methods, for that matter?

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

@kibertoad yep, altho what's more common is replacing builtins with broken implementations.

@kibertoad
Copy link

@ljharb during ESLint run?.. if application does that, it's not going to affect ESLint run. And if ESLint run overrides whatever choices application made to work and break the application, ESLint is going to be removed from the project very quickly.

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

I agree that it's very unlikely that an eslint config, or an eslint plugin, will modify globals during an eslint run, but it's not impossible.

@kibertoad
Copy link

There is an almost infinite set of not impossible crazy things a user might do. However, since ESLint is executed from CLI, it is exceedingly improbable anyone will invent a crazy ESLint hook just to break their own pipeline. And if they go to that length, which cannot be accidental, why stop them?

@43081j
Copy link
Contributor Author

43081j commented Jan 8, 2024

indeed @kibertoad is correct. by keeping this package around, we're covering the case of a consumer installing an eslint plugin in parallel to this config which mutates globals in an invalid way

not impossible, true, but probably hasn't been seen in your lifetime (would love to see an example if i'm wrong there).

similarly, all other globals, prototypes and even the ESLint API you use can have been modified the same way. do you intend on wrapping each of those in your own functions? no more direct interaction with the ESLint API, no more array prototype methods, no global namespaces.

@ljharb
Copy link
Collaborator

ljharb commented Jan 8, 2024

In general, robustness matters even if other parts of your stack are not robust. I agree that for eslint things it's much less of a concern.

@kibertoad
Copy link

@ljharb Would you accept this PR, then?

@ljharb ljharb changed the title chore: remove object.entries dependency remove object.entries dependency Jan 28, 2024
ljharb and others added 3 commits January 27, 2024 21:07
@ljharb ljharb merged commit 51a37d0 into airbnb:master Jan 28, 2024
43 checks passed
@43081j 43081j deleted the wwib branch January 28, 2024 09:33
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.

4 participants