Hand-roll faster and safer version of import/no-restricted-paths
#7593
Conversation
Detection works, whitelisting doesn't
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7593 +/- ##
=======================================
Coverage 72.72% 72.72%
=======================================
Files 1220 1220
Lines 38126 38126
Branches 7170 7170
=======================================
Hits 27727 27727
Misses 10399 10399 ☔ View full report in Codecov by Sentry. |
| "sidebar", | ||
| "pageScript", | ||
| ], | ||
| allowedGlobs: ["**/messenger/**", "**/*.scss*"], |
There was a problem hiding this comment.
⚡️ Improvement: Glob support
| message: `Cross-context imports break expectations. Shared components should be in shared folders. | ||
| Solution 1: Keep both importing and imported modules in the same context (shared or @/${exporter}). | ||
| Solution 2: Use the Messenger if they are in the correct context. | ||
| Solution 3: Propose a clearly-shared component within the context, like we do for Messenger and *Types files.`, |
There was a problem hiding this comment.
⚡️ Improvement: shorter message, moved to https://github.com/pixiebrix/pixiebrix-extension/wiki/Contexts#eslint
| `../${exporter}/pageEditorTypes.ts`, | ||
| `../${exporter}/runBlockTypes.ts`, | ||
| `../${exporter}/extensionPoints/formStateTypes.ts`, | ||
| `../${exporter}/tabs/editTab/dataPanel/dataPanelTypes.ts`, |
There was a problem hiding this comment.
⚡️ Improvement: type imports are automatically detected and allowed.
| // All of these files cannot import `from` (exclude self-imports) | ||
| target: | ||
| exporter === "contentScript" | ||
| ? `./src/!(${exporter}|bricks|starterBricks)/**/*` // Temporary: Bricks and starterBricks are implicitly run from CS |
There was a problem hiding this comment.
👋 Change: this gave free rein to bricks. If "bricks are implicitly run from CS", they should be under @/contentScript so that we can enforce this rule on all imports.
| @@ -29,7 +29,6 @@ import { recreateDB as recreateBrickDB } from "@/registry/packageRegistry"; | |||
| import { | |||
| recreateDB as recreateEventDB, | |||
| clear as clearEvents, | |||
| // eslint-disable-next-line import/no-restricted-paths -- safe import because IDB is shared resource | |||
| } from "@/background/telemetry"; | |||
There was a problem hiding this comment.
@/background, that's why we have this lint rule
There was a problem hiding this comment.
Agreed. But why is it not getting flagged by the new rule? We should be moving this, shouldn't we?
There was a problem hiding this comment.
It is, but the rule is set to be a warning. This inline exclusion was not "mandatory" so the linter continues without it.
There was a problem hiding this comment.
Just wanted to be sure that there wasn't something broken here before I approved. I overlooked that it was set to "warn"
Syntax error
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
|
@fregante fwiw if this actually is a faster 1:1 replacement for the import rule, i'd love an upstream PR :-) |
|
Unfortunately this rule takes a lot of shortcuts and just compares the top-level folder. It also changes how the comparison is done (one rule per folder vs "list of folders that can't import from each other") |
…he rendered document (#7507) * add stylesheets to the document view * add a document test * default input for document view component --------- Co-authored-by: Ben Loe <[email protected]>
What does this PR do?
import/no-restricted-pathswithno-restricted-importseslint-config-pixiebrix#35import/no-restricted-pathswith hand-rolled version? eslint-config-pixiebrix#123👆 😅 it was so slow that I opened two issues for the same thing over the years.
Before (2m 20s)
After (1m 30s)
Checklist