Chore/8201 deprecate json tokens #8246
Conversation
weronikaolejniczak
force-pushed
the
chore/8201-deprecate-json-tokens
branch
from
04461a6 to
5cd5dd5
Compare
weronikaolejniczak
force-pushed
the
chore/8201-deprecate-json-tokens
branch
from
5cd5dd5 to
6a751f9
Compare
weronikaolejniczak
force-pushed
the
chore/8201-deprecate-json-tokens
branch
from
6a751f9 to
b8d7a96
Compare
There was a problem hiding this comment.
This is great, thank you @weronikaolejniczak (also for the patience!)
I got this working locally without issues 👌
I added some comments, and I included [non-blocking] in some of them, but thinking about it, all comments are non-blocking because they're mostly questions and ideas that could turn into improvements…
Let me know what you think and we can get this finally merged 🙂
| return { | ||
| ImportDeclaration(node) { | ||
| allPatterns.forEach(({ pattern, message }) => { | ||
| const regex = new RegExp(pattern.replace('*', '.*')); |
There was a problem hiding this comment.
I need to ask this because I'm not well-versed with globs — will this pattern.replace('*', '.*') play nicely with any custom user patterns? (I think yes but I'm not sure)
There was a problem hiding this comment.
This implementation will not work with any custom user pattern.
- It will only work with wildcards (
*) but not globstars (**) - this will result in an invalid regular expression after the replacement. - What's more, it could lead to incorrect matches if special characters are unescaped, e.g. in
'@elastic/eui/dist/eui_theme_*.json'I didn't escape the dot, so it will be treated as a regular operator - it will match any single character - and will match'@elastic/eui/dist/eui_theme_borealis_darkkjson'.
It's not a fully glob implementation but a simplification for the use case we're working with.
For a comprehensive implementation we could leverage a globbing library like the one you mentioned (multimatch; or we could go for more performant, safer micromatch that covers the functionality of both minimatch and multimatch).
What do you think?
There was a problem hiding this comment.
Considering the rule could actually be used outside of our specific use case, I think it's worth it adding a library. I like the micromatch one you found (I didn't know it)!
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @acstll |
| return { | ||
| ImportDeclaration(node) { | ||
| allPatterns.forEach(({ pattern, message }) => { | ||
| const regex = new RegExp(pattern.replace('*', '.*')); |
There was a problem hiding this comment.
Considering the rule could actually be used outside of our specific use case, I think it's worth it adding a library. I like the micromatch one you found (I didn't know it)!
Summary
I added a new rule to our ESLint plugin,
no-restricted-eui-imports, that won't conflict with the inbuiltno-restricted-importsrule and will allow us to define several error levels at once. I.e. we want some imports to be marked as warning and not an error, as it's done in Kibana.Context:
The JSON token imports in
@kbn/eslint/module_migrationneed to be removed as well:https://github.com/elastic/kibana/blob/212b1926743ca5821992c2877d9c68f621e1875e/packages/kbn-eslint-config/.eslintrc.js#L131-L140
QA
yarn workspace @elastic/eslint-plugin-eui pack"@elastic/eslint-plugin-eui": "file:path/to/the/package.tgz"yarnand bootstrap:yarn kbn bootstrap --no-validateUnit tests:
yarn workspace @elastic/eslint-plugin-eui test