Added no-directory-barrel-imports rule #17526
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Alternatively we could update |
|
Devhost visualization test reporter: |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
sebavan
left a comment
sebavan
left a comment
There was a problem hiding this comment.
LGTM but not approving to give time for others to review
|
The build failed proving the rule is working out correctly !!! :-) @marns can you update the branch with the fix so it will be ready to merge |
|
Devhost visualization test reporter: |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
… some false positives in tools package with different bundling behavior
…r and a file with the same name exist in the same. specifically resolves abstractEngine.ts vs AbstractEngine/ which are ambiguous on case-insensitive file systems i.e. Mac OS
|
Devhost visualization test reporter: |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
|
@sebavan Updated and passing with a couple of fixes for false positives, including the special case of abstractEngine vs AbstractEngine on Mac 😅 |
| const importPath = node.source.value as string; | ||
|
|
||
| // Skip relative imports and node_modules imports | ||
| if (importPath.startsWith(".") || importPath.startsWith("@") || !tsConfig?.compilerOptions?.paths) { |
There was a problem hiding this comment.
should it not run for relative imports ?
There was a problem hiding this comment.
I was wondering about this one as well.
There was a problem hiding this comment.
I think this PR is for consumers of BJS, not internal imports inside BJS. Will consumers use relative imports?
There was a problem hiding this comment.
The "consumer" in this case being inspector-v2.
There was a problem hiding this comment.
It's still wrong though within core for example to do import { StandardMaterial } from "../Materials", and it seems like it would be nice if this rule caught that as well (unless I'm just misunderstanding the code).
There was a problem hiding this comment.
It only checked path mapped imports since these are the ones that produce the ambiguity / error, via pathTransform.ts:transformPackageLocation
I tried applying it to relative paths also and found only one violation, so maybe good to apply broadly if that's a general policy?
5 participants
Added a new ESLint rule
babylonjs/no-directory-barrel-importsthat detects imports resolving to directories with index files when using path mappings.Related: #17525
Rule by Claude