feat: no-token-import rule #103889
feat: no-token-import rule #103889
Conversation
github-actions
bot
added
the
Scope: Frontend
| import invariant from 'invariant'; | ||
| import typescript from 'typescript-eslint'; | ||
|
|
||
| import * as sentryScrapsPlugin from './static/eslint/eslintPluginScraps/index.mjs'; |
There was a problem hiding this comment.
we can’t to .ts because that would require a build-step for the eslint config 😭
| transform: { | ||
| '^.+\\.jsx?$': ['babel-jest', babelConfig as any], | ||
| '^.+\\.tsx?$': ['babel-jest', babelConfig as any], | ||
| '^.+\\.mjs?$': ['babel-jest', babelConfig as any], |
There was a problem hiding this comment.
jest can’t read mjs so we now transform it with babel like all other ts files
There was a problem hiding this comment.
jest is just so weird. structuredClone exists in the node version we use but I think the jsdom env doesn’t see it? it exists in node:util to “polyfill” it but ts doesn’t know that. What a mess!
There was a problem hiding this comment.
you could change the test env for this test, doesn't need to be jsdom https://jestjs.io/docs/configuration#testenvironment-string but some of the beforeEach etc might all break i guess
There was a problem hiding this comment.
Yeah I tried that and was hit with window is not defined or something similar :/
…ken-files-anywhere-besides-theme
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }, | ||
|
|
||
| create(context) { | ||
| const importerIsInAllowedDir = context.filename?.includes(EXCEPT_DIR_NAME); |
There was a problem hiding this comment.
Bug: Path matching allows unintended directories
The includes() check for EXCEPT_DIR_NAME matches any path containing the substring 'static/app/utils/theme', which incorrectly allows imports from unintended directories like static/app/utils/theme-old/ or static/app/utils/themeConfig/. This violates the rule's intent to only allow imports from within the static/app/utils/theme/ directory. A proper directory boundary check is needed.
This ensures `.js` and `.mjs` files are type-checked (with jsdoc comments) per default. We’ll need this more when e.g. writing custom eslint rules, as they have to be `.mjs` because we don’t have a typescript transform pipeline and eslint can’t natively read ts. see: - #103889
…ken-files-anywhere-besides-theme
it's on per default now
TkDodo
deleted the
tkdodo/feat/de-555-we-forbid-imports-of-token-files-anywhere-besides-theme
branch
This PR adds our first custom eslint rule,
@sentry/scraps/no-token-import, which disallows importing our generated tokens except from withinstatic/app/utils/theme.I opted for a custom estlint rule instead of using
no-restricted-importbecause:no-restricted-importscan report errors everywhere, and then we’d need to either disable it inside the file we want to allow, or have eslint overrideno-restriced-importsfor our allowed location. But, if we do that, we also disable all the other restricted imports. Yes, this would be solvable by extracting them to a common object and re-spreading them, but I think the custom rule is much clearer and has a better error message. Focused rule are better than generic ones imo.