Fix bug where package with sub-import would require extension #32
Conversation
| @@ -8,7 +8,7 @@ const path = require("path") | |||
| const fs = require("fs") | |||
| const mapTypescriptExtension = require("../util/map-typescript-extension") | |||
| const visitImport = require("../util/visit-import") | |||
| const packageNamePattern = /^(?:@[^/\\]+[/\\])?[^/\\]+$/u | |||
| const packageNamePattern = /^(?:@[^/\\]+[/\\])?(?:[^./\\]+[/\\])*[^/\\]+$/u | |||
There was a problem hiding this comment.
good to have a comment to explain the regex - regex is always easy to write but hard to read :)
There was a problem hiding this comment.
Will do!
FYI old regex was matching:
optional @scope/ followed by non-slash characters
which excluded imports like @scope/pkg/dir
new one:
optional @scope/ followed by an optional number of groups containing 1 or more characters ending with a slash followed by by non-slash characters
There was a problem hiding this comment.
Here is a quick before and after using my favourite regex renderer 😄
Before: /^(?:@[^/\\]+[/\\])?[^/\\]+$/u
After: /^(?:@[^/\\]+[/\\])?(?:[^./\\]+[/\\])*[^/\\]+$/u
It does looks like the change should work too:
| filename: fixture("test.ts"), | ||
| code: "import {test} from '@typescript-eslint/parser/index'", | ||
| options: ["always"], | ||
| }, |
There was a problem hiding this comment.
if the ./index was defined in its exports, the ext is disallowed, else it's always required in esm.
"exports": {"./index": "...."}There was a problem hiding this comment.
Ah, true, that said, I cannot figure out how to trigger a falling test, like if in code as reported in issue if you have import { keysPBM, supportedKeys } from "@libp2p/crypto/keys"; it will report an error and put extension even though it shouldn't. But cannot figure out how to make falling test case for that
|
Are you planning to merge this PR in v16 major ? @aladdin-add |
|
It can be, but it can also be released later in v16.x.x - given it is a non-breaking bugfix. I kind of lost the context, will be back to review this later. :) |
|
Hey @aladdin-add I'd love for this PR to get merged! Is it ready to merge or are you still waiting for your feedback to be addressed? I'm happy to make a new PR with the added requested comments if it would be helpful! |
|
I think a better solution here might be to change An example of what I mean can be found in #132 |
|
This should have been resolved by #132 |
Should fix #21