Replace TS extensions in generated declaration files #556

Andarist · 2023-05-18T08:31:36Z

No description provided.

changeset-bot · 2023-05-18T08:31:39Z

🦋 Changeset detected

Latest commit: 6716bdc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@preconstruct/cli	Minor

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

Andarist · 2023-05-18T08:31:58Z

packages/cli/src/build/__tests__/build.ts

@@ -2191,3 +2191,54 @@ test("correct default export using mjs and dmts proxies with moduleResolution: b
  `);
  expect(node.stderr.toString("utf8")).toMatchInlineSnapshot(`""`);
 });
+
+test("replaces ts extensions in module specifiers within generated declarations", async () => {


i'll add more tests after we agree on the exact implementation

Andarist · 2023-05-18T08:34:06Z

packages/cli/src/rollup-plugins/typescript-declarations/common.ts

+          const cachedVisitModuleSpecifier = memoize(
+            visitModuleSpecifier
+              ? (moduleSpecifier) =>
+                  replaceTsExtensions(visitModuleSpecifier(moduleSpecifier))
+              : replaceTsExtensions
+          );


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 · 2023-05-18T08:35:21Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (6860429) 92.12% compared to head (379e88b) 92.15%.

❗ Current head 379e88b differs from pull request most recent head 6716bdc. Consider uploading reports for the commit 6716bdc to get more accurate results

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

Impacted Files	Coverage Δ
...c/rollup-plugins/typescript-declarations/common.ts	`87.09% <ø> (ø)`
...tions-with-imported-module-specifiers-replacing.ts	`96.15% <100.00%> (ø)`
...rc/rollup-plugins/typescript-declarations/index.ts	`96.10% <100.00%> (-0.05%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Andarist · 2023-05-18T08:36:31Z

packages/cli/src/rollup-plugins/typescript-declarations/common.ts

+                  ".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",


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...

Andarist · 2023-05-18T08:38:28Z

packages/cli/src/rollup-plugins/typescript-declarations/common.ts

+                  ".mts": ".mjs",
+                  ".cts": ".cjs",


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.

Andarist · 2023-05-18T09:06:39Z

packages/cli/src/rollup-plugins/typescript-declarations/common.ts

+            return moduleSpecifier.replace(
+              /(\.d)?\.(ts|tsx|mts|cts)$/,
+              (match) => {


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.

emmatown

Two things:

The implementation here would break if I have a file named something.ts.ts and import it with ./something.ts
I'd like to only have this behaviour under the importsCondition flag 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:

preconstruct/packages/cli/src/rollup-plugins/typescript-declarations/get-used-declaration-with-replacing.ts

Lines 48 to 50 in 1147fee

    
           if (imported[0] !== "#") { 
        
             return imported; 
        
           }

Andarist · 2023-05-18T10:14:32Z

The implementation here would break if I have a file named something.ts.ts and import it with ./something.ts

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.

I'd like to only have this behaviour under the importsCondition flag for now since that's the only place we currently replace module specifiers in TypeScript declarations

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 importConditions really and one can use allowImportingTsExtensions: true

So with those two things in mind, the implementation should really just be delete these three lines:

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 importConditions)

emmatown · 2023-05-18T10:24:32Z

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.

Yeah, this is what I'm worried about, wdyt if we do it in both importsConditions and onlyEmitUsedTypeScriptDeclarations?

Andarist · 2023-05-18T10:27:23Z

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.

emmatown · 2023-05-18T10:34:46Z

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 onlyEmitUsedTypeScriptDeclarations will be the default soon. (importsConditions will probably take longer to get right)

Andarist · 2023-05-18T11:06:31Z

@emmatown pushed out the requested changes

emmatown

Could you add tests for a few cases:

Importing a .d.ts file with .d.ts (as in a .d.ts file in src)
Referencing a non-.d.ts .ts file with .ts in a .d.ts file
Using #imports in a .d.ts file
Using #imports in a .ts file with onlyEmitUsedTypeScriptDeclarations but not importsConditions (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)

Andarist · 2023-05-18T11:17:26Z

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 😅

…-in-declarations

Andarist · 2023-05-19T10:40:10Z

@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 src/**.d.ts within program.emit and that's the only place where I know how to get access to the TransformationContext that is required by visitEachChild.

I think this PR is in good shape right now and further improvements could be made in a separate PR, WDYT?

Replace TS extensions in generated declaration files

ea9649e

Andarist requested a review from emmatown May 18, 2023 08:31

Andarist commented May 18, 2023

View reviewed changes

emmatown reviewed May 18, 2023

View reviewed changes

Andarist added 3 commits May 18, 2023 12:57

only do the replacing with experimental flags

1f18bd6

move tests

cf63b6a

add changeset

9ccd8b5

emmatown reviewed May 18, 2023

View reviewed changes

Andarist added 3 commits May 19, 2023 10:02

Merge remote-tracking branch 'origin/main' into replace-ts-extensions…

8fe4c26

…-in-declarations

replace dts with js

622c1ec

add a test for imports replacing without importConditions

6716bdc

emmatown approved these changes May 19, 2023

View reviewed changes

emmatown merged commit 908c43e into main May 19, 2023
8 checks passed

emmatown deleted the replace-ts-extensions-in-declarations branch May 19, 2023 10:43

github-actions bot mentioned this pull request May 19, 2023

Version Packages #555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace TS extensions in generated declaration files #556

Replace TS extensions in generated declaration files #556

Andarist commented May 18, 2023

changeset-bot bot commented May 18, 2023 •

edited

Andarist May 18, 2023

Andarist May 18, 2023

codecov bot commented May 18, 2023 •

edited

Andarist May 18, 2023

Andarist May 18, 2023

Andarist May 18, 2023

emmatown left a comment •

edited

Andarist commented May 18, 2023

emmatown commented May 18, 2023

Andarist commented May 18, 2023

emmatown commented May 18, 2023

Andarist commented May 18, 2023

emmatown left a comment

Andarist commented May 18, 2023

Andarist commented May 19, 2023

Replace TS extensions in generated declaration files #556

Replace TS extensions in generated declaration files #556

Conversation

Andarist commented May 18, 2023

changeset-bot bot commented May 18, 2023 • edited

🦋 Changeset detected

Andarist May 18, 2023

Choose a reason for hiding this comment

Andarist May 18, 2023

Choose a reason for hiding this comment

codecov bot commented May 18, 2023 • edited

Codecov Report

Andarist May 18, 2023

Choose a reason for hiding this comment

Andarist May 18, 2023

Choose a reason for hiding this comment

Andarist May 18, 2023

Choose a reason for hiding this comment

emmatown left a comment • edited

Choose a reason for hiding this comment

Andarist commented May 18, 2023

emmatown commented May 18, 2023

Andarist commented May 18, 2023

emmatown commented May 18, 2023

Andarist commented May 18, 2023

emmatown left a comment

Choose a reason for hiding this comment

Andarist commented May 18, 2023

Andarist commented May 19, 2023

changeset-bot bot commented May 18, 2023 •

edited

codecov bot commented May 18, 2023 •

edited

emmatown left a comment •

edited