[eslint-plugin] support validImports options for all rules#1085
Conversation
facebook-github-bot
added
the
CLA Signed
| { | ||
| code: 'export const vars = stylex.defineVars({});', | ||
| code: ` | ||
| import stylex from 'stylex'; |
There was a problem hiding this comment.
The sideeffect of createImportTracker refactoring is that import statements must now be present in the file, not just specifying it in validImports option as we now collect import statements and check against the collected values.
Technically a breaking change but should not affect real code as imports should exist anyway.
| reportErrors(node); | ||
| }, | ||
| 'Program:exit'() { | ||
| importTracker.clear(); |
There was a problem hiding this comment.
I'm not too sure this cleanup is strictly necessary, but aligned it with packages/@stylexjs/eslint-plugin/src/stylex-sort-keys.js which does clean up.
| .forEach(parseStylexImportStyle); | ||
| // stylex.create can only be singular variable declarations at the root | ||
| // of the file so we can look directly on Program and populate our set. | ||
| .filter( |
There was a problem hiding this comment.
we need to ensure we collect import statements before variable detection happens, so this can't happen in ImportDeclaration and needs to be done in Program
| } | ||
|
|
||
| return { | ||
| ImportDeclaration: handleImportDeclaration, |
There was a problem hiding this comment.
Because of the conflict with ImportDeclaration type, function name cannot be ImportDeclaration so it was named handleImportDeclaration and aliased as ImportDeclaration here.
| // Possible strings where you can import stylex from | ||
| // | ||
| // Default: ['@stylexjs/stylex'] | ||
| // Default: ['stylex', '@stylexjs/stylex'] |
There was a problem hiding this comment.
Existing code already had stylex in validImports by default so I aligned the default for all rules and reflected it in the docs too.
javascripter
requested review from
Jta26,
mellyeliu,
necolas and
vincentriemer
as code owners
necolas
left a comment
necolas
left a comment
There was a problem hiding this comment.
Thanks this looks great. There's just some minor changes requested about test import syntax and removing support for default export, since there isn't one in the runtime.
necolas
left a comment
necolas
left a comment
There was a problem hiding this comment.
LGTM. I think as you mentioned, we can restore the default import support for internal use case since it's a one line change. Thanks for this great PR!
necolas
force-pushed
the
eslint-valid-imports
branch
from
bee2b7e to
8985f91
Compare
3 participants
What changed / motivation ?
Continuing on #1039, this PR adds
validImportsoption to all eslint rules with support for both the string and object syntax uniformly, for RSD compatibility and other custom aliases.I've also updated API docs and the README to document the options. I expect documentation rework to be made in another PR (pending #1057) so made the change minimal.
Main changes:
validImportsoption to all rulessrc/utils/createImportTracker.jsto reduce duplicationLinked PR/Issues
Fixes #1056
Additional Context
I ran the following tests locally and confirmed existing and new tests are passing:
npm run testScreenshots, Tests, Anything Else
Pre-flight checklist
Contribution Guidelines