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
Comments
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. |
Thanks for the report. The fact that 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. |
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 |
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! |
There is also an issue with |
just to provide an example: https://github.com/benmosher/eslint-plugin-import/blob/v2.21.2/docs/rules/no-cycle.md uses this is the |
https://github.com/benmosher/eslint-plugin-import/blob/055389d425caae7219659fb97b6d0b992d2f1aaa/README.md#importcache also allows the string There's also tons of internal configs in companies that are using functions and regex literals in their JS configs. |
I was surprised this use of
In the no-cycle docs, it mentions ESLint uses ajv to validate rules' JSON schemas, and its current version passes |
@btmills aha, thanks, that does explain it. It's been 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 |
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 |
* ✨ release 4.3.5 * 🔒 lock eslint version eslint/eslint#13427
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 |
This is breaking even Neutrino, effectively blocking people to start new projects without hiccups. Shouldn't this be a priority? |
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 |
@boeric +1 on your suggestion |
Yea, I guess having integration tests with popular configs/plugins can be done to address these changes in the future. +1 for that.
|
Any update on this issue? |
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. |
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. |
@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 In an effort to get our users unblocked, I've submitted #13435 that will replace Our position on We will plan on switching to a strict integer check in the next major release to ensure this bug doesn't continue. |
@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. |
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
The text was updated successfully, but these errors were encountered: