Replace TS extensions in generated declaration files#556
Conversation
🦋 Changeset detectedLatest commit: 6716bdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| expect(node.stderr.toString("utf8")).toMatchInlineSnapshot(`""`); | ||
| }); | ||
|
|
||
| test("replaces ts extensions in module specifiers within generated declarations", async () => { |
There was a problem hiding this comment.
i'll add more tests after we agree on the exact implementation
| const cachedVisitModuleSpecifier = memoize( | ||
| visitModuleSpecifier | ||
| ? (moduleSpecifier) => | ||
| replaceTsExtensions(visitModuleSpecifier(moduleSpecifier)) | ||
| : replaceTsExtensions | ||
| ); |
There was a problem hiding this comment.
Since allowImportingTsExtensions: true is allowed with just any moduleResolution type ( microsoft/TypeScript#52230 ), I think it's the best/easiest to just apply this logic at all times.
We could cross-reference if that option is enabled but it seems like an overcomplication to me. If it's not enabled then typechecking process would complain about it and it doesn't quite matter if we replace those or not when generating declaration files
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
==========================================
+ Coverage 92.12% 92.15% +0.02%
==========================================
Files 41 40 -1
Lines 1981 1962 -19
Branches 585 579 -6
==========================================
- Hits 1825 1808 -17
+ Misses 146 144 -2
Partials 10 10
☔ View full report in Codecov by Sentry. |
| ".d.ts": ".js", | ||
| ".d.tsx": ".js", // `.d.tsx` is not quite a thing but the current regex matches it, so it's included here for completeness | ||
| ".d.mts": ".mjs", | ||
| ".d.cts": ".cjs", |
There was a problem hiding this comment.
Nowadays (?) it's always allowed to import declaration files with an explicit extension but I have a feeling that it's best to just replace those extensions with their JS counterparts. This way it should support older TS versions as well. I didn't quite recheck if older TS versions would trip over those declaration extensions in declarations files but it's likely and replacing feels safe...
| ".mts": ".mjs", | ||
| ".cts": ".cjs", |
There was a problem hiding this comment.
I'm not quite sure how this would behave in older TS versions. It feels like a safe bet that those cases are not that popular today anyway and that we might triage the potential issues if somebody raises them. It also seems quite fair to say that using those extensions might require TS 4.8+ (or something like that) for package consumers.
| return moduleSpecifier.replace( | ||
| /(\.d)?\.(ts|tsx|mts|cts)$/, | ||
| (match) => { |
There was a problem hiding this comment.
I also decided to replace those with runtime equivalents because that works in older TS versions and it seems to be more compatible with the stricter rules imposed by type:module and stuff. While we don't really support type:module yet, we will at some point. So to reduce conditional logic for the future, I just went with replacing - instead of stripping those away.
There was a problem hiding this comment.
Two things:
- The implementation here would break if I have a file named
something.ts.tsand import it with./something.ts - I'd like to only have this behaviour under the
importsConditionflag for now since that's the only place we currently replace module specifiers in TypeScript declarations
So with those two things in mind, the implementation should really just be delete these three lines:
I guess, I could try to check if sources for those things exist and only replace them conditionally. I'll look into it - I rechecked that this is allowed by TS and it doesn't trip over this in its module specifier validation logic etc.
If we are worried about some unintended consequences related to this (although... it might be hard to verify if there are any even after shipping if we can't figure out them now), I think this should be a separate experimental flag. The behavior itself is not related anyhow to
Nice, I verified that it works so I could reuse that logic there (or rely on it if we'll do it only in combination with |
Yeah, this is what I'm worried about, wdyt if we do it in both |
|
How is that better than using a different experimental flag? Unless you treat those two modes as things that will become non-experimental soon-ish and as such this replacing logic will also become enabled at all times soon-ish. |
Yeah, I'm thinking |
|
@emmatown pushed out the requested changes |
emmatown
left a comment
There was a problem hiding this comment.
Could you add tests for a few cases:
- Importing a
.d.tsfile with.d.ts(as in a.d.tsfile insrc) - Referencing a non-
.d.ts.tsfile with.tsin a.d.tsfile - Using
#importsin a.d.tsfile - Using
#importsin a.tsfile withonlyEmitUsedTypeScriptDeclarationsbut notimportsConditions(with this new behaviour, I would expect it to be replaced
(I think to make all that work right, some changes will be required, particularly, we need to parse and transform .d.ts files)
|
Sure, I'll get to it - but it might take me a few more days. For now, I need to switch to some other work 😅 |
|
@emmatown I addressed half of your list. I still need to figure out how to properly transform declaration files. I tried a bunch of stuff but so far I can't reliably make TS to do anything for I think this PR is in good shape right now and further improvements could be made in a separate PR, WDYT? |
No description provided.