fix: Forbid self package import #1498
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
==========================================
- Coverage 45.77% 45.76% -0.02%
==========================================
Files 517 517
Lines 35116 35162 +46
Branches 8792 8802 +10
==========================================
+ Hits 16076 16092 +16
- Misses 18989 19019 +30
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
| "devDependencies": { | ||
| "eslint-plugin-deephaven": "file:./plugin" | ||
| }, |
There was a problem hiding this comment.
Looks like you don't need to do it this way. You can import the plugin directly
https://eslint.org/docs/latest/extend/custom-rule-tutorial#step-7-bundle-the-custom-rule-in-a-plugin
There was a problem hiding this comment.
This doesn't work if you try to add it to an array of plugins since the plugins array only supports string names. Granted the current config only has 1 plugin, this will be more future proof.
That said if you find a way to get it working alongside other plugins, I'd love to see how. I was not able to get it working.
There was a problem hiding this comment.
@mattrunyon @mofojed Thinking this PR may be a cleaner option
https://github.com/deephaven/web-client-ui/pull/1499/files
| return null; | ||
| } | ||
|
|
||
| return `@deephaven/${packageFolderName}`; |
There was a problem hiding this comment.
One thing I'm not crazy about is this rule is not generic; even when we include this eslint config from Enterprise it won't technically be correct. We should also be using the path package to resolve paths rather than manipulating the string directly, as there's probably some edge cases you're missing here (e.g. what if I had Enterprise using this eslint config and it was in a folder like ~/deephaven/packages/app/..., then it would think the package name was @deephaven/app).
Getting the package name from the package.json should be doable, though I do also wonder about performance and how we can cache results (since this will be called on every import in every file, don't want to have to traverse and read package.json every time...). I'd rather check in the fix first without the eslint rule than check it in with specific @deephaven logic.
There was a problem hiding this comment.
@mofojed This "shouldn't" ever actually be used in enterprise since it doesn't really apply. It is currently consumed via the top level Community package so doesn't get pulled in with the shared config. It really is scoped to @deephaven community packages. Does this alleviate any of your concern?
There was a problem hiding this comment.
I'm actually second guessing the approach of using this plugin now. Since it's not actually configured in the shared package and not really useful elsewhere, maybe the top level community config should be fully responsible for it. Would be easy to just get the list of packages and build a rule array there
|
Superceded by #1499 |
I tested this by running
npm run test:lint. It showed expected linting error before fixing SessionUtils. Fixing SessionUtils made error go away. Also tested in vscode. Error shows up in the editor as expected.fixes: #1497