-
-
Notifications
You must be signed in to change notification settings - Fork 26
fix: Ensure languageOptions.customSyntax is serializable #212
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
base: main
Are you sure you want to change the base?
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.
Pull Request Overview
This PR addresses a serialization issue with ESLint configuration by ensuring that languageOptions.customSyntax
containing functions can be properly serialized to JSON. This is necessary for ESLint features like configuration caching.
- Added
normalizeLanguageOptions()
method to handle serialization of customSyntax configurations - Enhanced test coverage for both unit and integration scenarios
- Updated JSDoc documentation for consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/languages/css-language.js | Implements serialization logic with normalizeLanguageOptions() method and helper function |
tests/languages/css-language.test.js | Unit tests for the new normalization functionality |
tests/plugin/eslint.test.js | Integration tests validating serialization behavior through ESLint |
* @param {CSSLanguageOptions} languageOptions The language options to normalize. | ||
* @returns {CSSLanguageOptions} The normalized language options. | ||
*/ | ||
normalizeLanguageOptions(languageOptions) { |
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.
[nitpick] The method modifies the input object by adding a toJSON
property. Consider returning a new object instead of mutating the input to avoid side effects and make the API more predictable.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Object.defineProperty(languageOptions, "toJSON", { | ||
value() { | ||
// Shallow copy | ||
const result = { ...languageOptions }; |
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.
Would it make sense to set the toJSON
method on a shallow clone of the argument (results
) so that the original object isn't modified?
Object.defineProperty(languageOptions, "toJSON", { | |
value() { | |
// Shallow copy | |
const result = { ...languageOptions }; | |
// Shallow copy | |
const result = { ...languageOptions }; | |
Object.defineProperty(result, "toJSON", { | |
value() { |
We'd need to change the return value of the function as well and update some unit tests.
* @param {Record<string,any>} object The object to process. | ||
* @returns {Record<string,any>} A copy of the object with all functions replaced by true. |
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 looks like this function also returns arrays and non-object values depending on the type of the argument?
Prerequisites checklist
What is the purpose of this pull request?
Ensures that
languageOptions.customSyntax
is serializable by replacing all functions withtrue
. CSSTree allows functions in its config and we need to account for that to allow things like caching in ESLint.What changes did you make? (Give an overview)
CSSLanguage#normalizeLanguageOptions()
that adds atoJSON
method when thecustomSyntax
option is presentCSSLanguage
tests to validate the new functionality works.Related Issues
fixes #211
Is there anything you'd like reviewers to focus on?