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

Config clone before validating kills Object data (7.3.0) #13427

Closed
Enase opened this issue Jun 19, 2020 · 23 comments · Fixed by #13435
Closed

Config clone before validating kills Object data (7.3.0) #13427

Enase opened this issue Jun 19, 2020 · 23 comments · Fixed by #13435
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion patch candidate This issue may necessitate a patch release in the next few days regression Something broke

Comments

@Enase
Copy link

Enase commented Jun 19, 2020

  • ESLint Version: 7.3.0

The latest update breaks eslint-plugin-import and I believe some other eslint plugins due to configuration cloning by JSON.parse(JSON.stringify(Object))
related PR: #13034

Any plugin that use any global property like "Infinity" wont work properly.

JSON.parse(JSON.stringify(Infinity)) => null

Configuration example https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/imports.js#L236

@Enase Enase added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jun 19, 2020
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 19, 2020

oof, yes, JSON cloning is a terrible way to clone objects :-/

JSON cloning should never be used, especially since eslint config values can be regexes, Infinity, NaN, and other things that don't survive JSON serialization.

@nzakas
Copy link
Member

nzakas commented Jun 19, 2020

Thanks for the report. The fact that Infinity worked at all is actually a bug because it can only ever work in the JS format of config file and not in JSON or YAML, and we tried to make all the config file formats equivalent. Our intent was for rules to only have JSON-serializable configs for this reason. In fact, if people are using special JS values in their configs and we aren’t catching it, chances are there are other underlying bugs we aren’t aware of because the codebase assumes a config can be completely JSON serialized in a few places.

We did try a few other cloning techniques before deciding on this. If anyone has a suggestion for an alternative, we are happy to consider it.

We do need to decide if we should formally allow JS special values, though, as this was completely unintentional.

@btmills btmills added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion patch candidate This issue may necessitate a patch release in the next few days regression Something broke and removed triage An ESLint team member will look at this issue soon labels Jun 19, 2020
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 19, 2020

Lots of plugins take regexes as a config, which yes, only work in a JS config file.

If eslint wants to forbid non-JSON values in a config, then I'd expect it would be a validation error that's not left up to plugin authors, but that'd probably be a pretty disruptive breaking change.

I'm happy to make a PR that uses extend or node.extend or lodash.clonedeep to employ a proper deep copy algorithm, if that's desirable.

@anikethsaha
Copy link
Member

@ljharb we tried 1453116, maybe this can be helpful.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 19, 2020

sure, that would work, but i'm not sure why it'd be inlined instead of just being a normal dependency? (also, you'd want to use a WeakSet instead of a Set)

edit: ah, i see it's to wrap the "customizer" stuff. sure, that works fine!

@btmills
Copy link
Member

btmills commented Jun 19, 2020

@ljharb to help us estimate impact here, do you have some examples of plugin rules that use regexes in configs? In built-in rules like id-match, the regex is represented as a string so it's still serializable. If we're lucky, those might do the same.

@anikethsaha
Copy link
Member

There is also an issue with Infinity that is being converted into null as well I guess.

@nescalante
Copy link

nescalante commented Jun 19, 2020

just to provide an example: https://github.com/benmosher/eslint-plugin-import/blob/v2.21.2/docs/rules/no-cycle.md uses Infinity as a possible value, which breaks with the new approach

this is the Infinity issue that @anikethsaha is mentioning, which is I think the issue that everyone is facing since it is being used by airbnb eslint config

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 19, 2020

https://github.com/benmosher/eslint-plugin-import/blob/055389d425caae7219659fb97b6d0b992d2f1aaa/README.md#importcache also allows the string "∞" or the literal Infinity for "lifetime"; while no-cycle could and should add "∞", this is just one plugin in the ecosystem that might be relying on this.

There's also tons of internal configs in companies that are using functions and regex literals in their JS configs.

@btmills
Copy link
Member

btmills commented Jun 19, 2020

I was surprised this use of Infinity was being allowed by the rule's JSON schema before such that we only caught it now, so I did some digging.

      maxDepth:{
        description: 'maximum dependency depth to traverse',
        type: 'integer',
        minimum: 1,
      },

eslint-plugin-import/no-cycle's maxDepth option schema requires an integer type.

This includes cycles of depth 1 (imported module imports me) to Infinity, if the maxDepth option is not set.

There is a maxDepth option available to prevent full expansion of very deep dependency trees:
This is not necessarily recommended, but available as a cost/benefit tradeoff mechanism for reducing total project lint time, if needed.

In the no-cycle docs, it mentions Infinity as the default if maxDepth is unset but says that maxDepth is only needed in order to limit search depth for performance reasons. (Does this mean that removing , { maxDepth: Infinity } would maintain the existing behavior?)

ESLint uses ajv to validate rules' JSON schemas, and its current version passes NaN and Infinity for integer. That is expected to change in the next major, and until then, there's an unreleased new strictNumbers option to opt-in to the updated behavior.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 19, 2020

@btmills aha, thanks, that does explain it. It's been Infinity for like 2 years without issue, fwiw.

Yes, removing the explicit default would work, but the airbnb config prefers to never rely on defaults. If this isn't something eslint is going to fix quickly, i could certainly update eslint-plugin-import to also allow "∞" as a default, and then update the airbnb config to do so as well, but i'd rather not have to do that under time pressure.

@brettz9
Copy link
Contributor

brettz9 commented Jun 20, 2020

If JSON remains enforced, I think it'd be nice if there could be a recommended serialization process for the common special objects above-mentioned, and maybe even a utility for converting them back (e.g., regexes being expected in a string format like "/(abc.*)/i" and then converted to RegExp objects) (schemas therefor will presumably still need to fall to authors*). Then users could expect a more uniform experience.

*FWIW, draft 7 of json-schema at least allows for "regex" as a format though I've left a comment asking for delimited regex support to allow for flags as in the above example).

@aryzing
Copy link
Contributor

aryzing commented Jun 20, 2020

@ljharb to possibly help with the time pressure, if you do decide to go for allowing the "∞" string, the schema may end up looking something like

      maxDepth:{
        description: 'maximum dependency depth to traverse',
        type: ['integer', 'string'],
        minimum: 1,
        pattern: "^∞$",
      },

mistic100 added a commit to mistic100/Photo-Sphere-Viewer that referenced this issue Jun 20, 2020
afc163 added a commit to ant-design/ant-design that referenced this issue Jun 21, 2020
afc163 added a commit to ant-design/ant-design that referenced this issue Jun 21, 2020
* ✨ release 4.3.5

* 🔒 lock eslint version

eslint/eslint#13427
@ddolcimascolo
Copy link

Hey guys, what's the plan with this? Are we really saying to everyone using the Airbnb preset that they can't upgrade eslint anymore?

Thx

@lucasff
Copy link

lucasff commented Jun 21, 2020

This is breaking even Neutrino, effectively blocking people to start new projects without hiccups. Shouldn't this be a priority?

mtrenker added a commit to mtrenker/clean.dev that referenced this issue Jun 21, 2020
@boeric
Copy link

boeric commented Jun 21, 2020

Eslint is an awesome tool to ensure code style consistency, especially when paired with airbnb's stricter rules. The eslint-config-airbnb-base, with some 3 million weekly downloads, now breaks with 7.3.0 as pointed out. A humble suggestion for the future is to integrate tests that represents how eslint is used by the most popular repos in its ecosystem (of which eslint-config-airbnb-base is one) and not break compatibility until the next major rev

@ddolcimascolo
Copy link

@boeric +1 on your suggestion

@anikethsaha
Copy link
Member

Yea, I guess having integration tests with popular configs/plugins can be done to address these changes in the future.

+1 for that.

also, apologizes for this regression as this was introduced due to my PR.

@fernandolguevara
Copy link

Any update on this issue?

@nzakas
Copy link
Member

nzakas commented Jun 22, 2020

We are still discussing this.

It seems like an easy solution is for airbnb to replace Infinity with Number.MAX_SAFE_INTEGER. That should effectively be the same thing but is serializable and doesn’t require updates elsewhere in the ecosystem.

We dug in and found both —print-config and —cache do not work properly with the airbnb config right now because they both depend on serialization to work. So, I’d like to find a way to keep this change in to prevent any other configs from introducing special values that could also break this functionality.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 22, 2020

Given the rarity of users using print-config or cache, and considering this has been in the airbnb config for two years, it seems likely that there's a significant amount of the ecosystem that is unaware that rule schemas must be JSON-serializable values.

I implore you to reconsider and delay this restriction until the next semver-major, and consider this a missed opportunity from the v7.0.0 release.

@nzakas
Copy link
Member

nzakas commented Jun 22, 2020

@ljharb you've made comments in this thread that are untrue, from regular expressions being common in rule configs and the rarity of people using --print-config and --cache. I'd ask that you refrain from speculating about what features other users of the ecosystem make use of as it's distracting and may confuse those who aren't as familiar with ESLint. I completely understand that you are frustrated, but let's please stick to the facts.

In an effort to get our users unblocked, I've submitted #13435 that will replace Infinity with Number.MAX_SAFE_INTEGER in rule configs during validation (but not in other parts of the codebase that rely on serialization). This will effectively restore the v7.2.0 behavior. That means things like --cache and --print-config won't work correctly for any config including Infinity.

Our position on eslint-config-airbnb is that it should be updated to use Number.MAX_SAFE_INTEGER as soon as possible. The change we are making is to buy you some time to make that change. Please do so at your earliest convenience.

We will plan on switching to a strict integer check in the next major release to ensure this bug doesn't continue.

mikesprague added a commit to mikesprague/delivery-status that referenced this issue Jun 22, 2020
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 22, 2020

@nzakas it's fair that I don't have facts about how many people use these features; nor is it possible to have them, since enterprise usage of things like this are common. Airbnb, for example, has internal rules that use functions, and regexes, in their config, so that's one hard data point.

Thanks for that fix; that will at least fix the airbnb config and revert the breaking change, but it won't fix any users that may be relying on non-JSON-serializable config values (a large number of folks requesting rules on eslint-plugin-import, eslint-plugin-react, and eslint-plugin-jsx-a11y all expect and request regex literals to be usable in configs, for another data point).

I'm happy about the strict integer check in v8; hopefully it will also ensure that functions and regexes and Dates and other non-serializeable values are treated as invalid as well.

chalkygames123 added a commit to chalkygames123/front-end-template that referenced this issue Jun 23, 2020
noriaki added a commit to noriaki/react-timer-sandbox that referenced this issue Jun 24, 2020
dhoko pushed a commit to ProtonMail/proton-calendar that referenced this issue Jun 30, 2020
kaicataldo pushed a commit that referenced this issue Jul 3, 2020
* Revert "Fix: Replace Infinity with Number.MAX_SAFE_INTEGER (fixes #13427) (#13435)"

This reverts commit de77c11.

* Revert "Fix: clone config before validating (fixes #12592) (#13034)"

This reverts commit 7fb45cf.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 21, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion patch candidate This issue may necessitate a patch release in the next few days regression Something broke
Projects
None yet
Development

Successfully merging a pull request may close this issue.