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
Fix: clone config before validating (fixes #12592) #13034
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually finding where the validate
function is used, and I'm wondering if this is dead code. Please let me know if I'm missing something.
Additionally, we're using ajv
's useDefaults
option (see here), and I think cloning it in the validation function means that these default values won't be used by the rules. Can we clone the config at a higher level and then pass that to the validator function?
We'll also want to include some tests for this, though I don't think this PR should require any rule test changes. I think we'll want to test that a config object doesn't get mutated after a run of the linter.
Thanks for working on this! |
By higher level, you meant where we are reading/getting the config? then I think it needs to pass two configs to the validator one cloned and one original. cause only the cloned one will be validated. the original one will be passed to the rules |
Is there a reason we can't clone the config as soon as we have the full resolved config and use that for validation and then pass that to the rules? |
i will try that 👍 |
b35883f
to
aad0f64
Compare
@kaicataldo I updated the cloning to a higher level. Also, do you think it would be helpful to add a clone eslint/lib/cli-engine/config-array-factory.js Lines 803 to 809 in 76324ac
Like this if (configData) {
const clonedConfigData = lodash.cloneDeep(configData);
return this._normalizeConfigData(
clonedConfigData,
plugin.filePath,
`${importerName} » plugin:${plugin.id}/${configName}`
);
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It had been cloned when ajv's useDefaults
was introduced( see here
), but was removed somehow.
- I agreed it better to be in a higher level: reading config -> cloning config -> validating config.
- a test case is required!
thanks for working on this!
@aladdin-add can you check if the current one is fine ? let me know if this has to be changed. after that, I will add some tests |
I'd suggest it in something like |
I think the current one in this PR is something like this. earlier I planned to have it clone and validate in But I think the current one is the entry point for the config (not for the directives,) |
@kaicataldo can you please take a look if this is the fine place to clone? |
@kaicataldo can you take a look at this again? |
Apologies for the delay! I've been sick the last few weeks and am just getting caught up. |
aad0f64
to
b757bd8
Compare
@nzakas feel free to finish this or I can try to finish this as well if the final direction has been set whether to use Either way works for me. 👍 |
The direction has been set. If you're able to update this, that would be great as I don't have a lot of time. But if you can't finish it, then I can take it over. It just might take some time for me to finish. |
@nzakas So are we moving forward with
ok cool, I will try to finish this weekend then. |
Correct, and thanks 👍 |
Done 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction LGTM, thank you!
I have some small suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for sticking with this! We really appreciate it.
Changes have been addressed - Kai on vacation
@nzakas @mysticatea I think we need to revert this ? 7fb45cf#r40036492 Cause I guess users are using non serialized data for the config |
* | ||
* Refer https://github.com/eslint/eslint/issues/12592 | ||
*/ | ||
const clonedRulesConfig = rules && JSON.parse(JSON.stringify((rules))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately i never saw this PR; 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb We did try to avoid json cloning using this. But we tried using lodash cloneDeepWith with customizer but that had different issues like circular ref issue,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BREAKING CHANGE for a lot of plugins which support options via
regexes, Infinity, NaN, and other things that don't survive JSON serialization
For example: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/filename-case.md#ignore
…int#13034)" This reverts commit 7fb45cf.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Cloning the rule config and other configs before validating them as they get mutated with default values when used
useDefault : true
inagv
.I couldn't find any other entry point for this issue other than this, do let me know if any!
Is there anything you'd like reviewers to focus on?
fixes #12592