-
Notifications
You must be signed in to change notification settings - Fork 364
[eslint-plugin] Add transform-media-query rule and soft validation config #1301
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
| let lastMediaQueryWinsTransform = null; | ||
| try { | ||
| // $FlowFixMe Dynamic import for style-value-parser since it might not be available in all environments | ||
| const styleValueParser = require('style-value-parser'); |
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.
@mellyeliu Not sure if the import statement is safe here?
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.
hmm why cjs here? let's use esm import instead
workflow: benchmarks/perfComparison of performance test results, measured in operations per second. Larger is better.
|
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
mellyeliu
left a comment
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.
Looks good overall. Couple of nits and suggestions to start out
| type: 'problem', | ||
| docs: { | ||
| description: | ||
| 'Warn when media query syntax may not be correctly ordered due to parser limitations', |
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.
| 'Warn when media query syntax may not be correctly ordered due to parser limitations', | |
| 'Warn when media query syntax order is not properly enforced due to invalid media query syntax or parser limitations', |
| } | ||
|
|
||
| if (prop.computed) { | ||
| continue; |
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 fine for now, we'll eventually need to think of how to handle defineConsts computed keys here though
| `StyleX: ${messages.INVALID_MEDIA_QUERY_SYNTAX}\n` + | ||
| 'Media query order will not be respected. ' + | ||
| 'This could be due to invalid media query syntax or unsupported edge cases in style-value-parser.\n' + | ||
| `Error: ${error.message || error}`, |
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.
can we validate that this error message contains the erroring media query?
| throw new Error(messages.INVALID_MEDIA_QUERY_SYNTAX); | ||
| if (options.softMediaQueryValidation) { | ||
| console.warn( | ||
| `StyleX: ${messages.INVALID_MEDIA_QUERY_SYNTAX}\n` + |
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.
probably don't need to "StyleX: " prefix here
| z.logAndDefault( | ||
| z.boolean(), | ||
| options.softMediaQueryValidation ?? false, | ||
| false, |
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.
let's make this true for now
| const styles = stylex.create({ | ||
| main: { | ||
| color: { | ||
| '@media (min-width: 768px)': 'blue', |
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.
we require a default with every contextual style:
| '@media (min-width: 768px)': 'blue', | |
| default: 'yellow', | |
| '@media (min-width: 768px)': 'blue', |
| `, | ||
| errors: [ | ||
| { | ||
| message: /Media query order may not be respected/, |
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.
Curious why the rest of the error is not showing up here? Seems out of sync with the rule file itself
| import sortKeys from './stylex-sort-keys'; | ||
| import validShorthands from './stylex-valid-shorthands'; | ||
| import validStyles from './stylex-valid-styles'; | ||
| import validateMediaQueries from './stylex-validate-media-queries'; |
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.
Let's call this transform-media-queries or something similar
| "micromatch": "^4.0.5", | ||
| "postcss-value-parser": "^4.2.0" | ||
| "postcss-value-parser": "^4.2.0", | ||
| "style-value-parser": "*" |
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 believe style-value-parser is already added to workspaces at the root level so it's inlined instead, as it's a private package, see: https://github.com/facebook/stylex/blob/main/package.json#L20
What changed / motivation?
Introducing a new ESLint rule
stylex-validate-media-queriesthat validates media query syntax in StyleX declarations. The motivation is to provide early feedback to developers about invalid media queries during development and flag when media query order cannot be enforced.Key Changes:
stylex-validate-media-queriesrule that checks media query syntax instylex.create()andstylex.createTheme()callssoftValidationoption in the babel plugin that allows media query validation to be handled by ESLint instead of failing at build timeLinked PR/Issues
Fixes #1200
Tests
Configuration
The ESLint rule can be enabled in
.eslintrc:{ "rules": { "@stylexjs/stylex-validate-media-queries": "error" } }The babel plugin's soft validation mode can be enabled via:
Breaking Changes
None - this is an opt-in feature that doesn't affect existing functionality.
Pre-flight checklist
Contribution Guidelines