Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: declaration extensions should correspond to their js extension #268

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjpearson
Copy link

This is a partial fix for #138. (Wasn't sure if I should split it out into a new bug) It addresses the issue where changing the extension of the output does not affect extension of the declaration so there is a mismatch. It does not implement automatically choosing the extension based on the source file.

The other important thing is that .ts, .cts, .mts on the declaration files imply the existence of the file with the same basename: that is, index.d.ts says ‘the declaration file is for index.js’, a.d.cts is ‘for a.cjs’, and b.d.mts is ‘for b.mjs’.

https://www.typescriptlang.org/docs/handbook/modules/reference.html#file-extension-substitution

The change itself is relatively straightforward, but it does have a large impact, especially because mjs is the default extension for esm output. Perhaps a major bump would be necessary or hiding the behavior behind a flag?

@pi0 pi0 requested a review from danielroe December 13, 2024 20:51
"dist/dir-export.mjs",
"dist/foo.mjs",
"dist/foo.d.ts",
"dist/foo.d.mts",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case there is an existing foo.d.ts in the source, but the generated foo.d.mts will be used instead. Would it be good to copy foo.d.ts and rename it? The developer could rename it to foo.d.mts in the src dir, but that would not work if they are generating both cjs and esm.

"dist/bar/esm.mjs",
"dist/bar/esm.d.mts",
"dist/ts/test1.mjs",
"dist/ts/test2.mjs",
"dist/ts/test1.d.mts",
"dist/ts/test2.d.cts",
"dist/ts/test2.d.mts",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we have a test2.cts in the src. I think there are two ways to handle it:

  1. When a .cts or .mts file is found in the source, generate .cjs/.d.cts or .mjs/.d.mts pairs regardless of the ext option. This is the issue described by Declarations not genered to .mts and .cts files #138. Another variation would be to only override ext if it were not explicitly set.
  2. Always use the ext option and treat .mts, .cts, and .ts files in src/ identically.

It's just important that the declaration and js file match, so you don't end up with foo.cjs and foo.d.mts

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.29%. Comparing base (9000888) to head (bd52c24).
Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
src/loaders/js.ts 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   82.86%   80.29%   -2.58%     
==========================================
  Files          12       12              
  Lines         852      888      +36     
  Branches      133      190      +57     
==========================================
+ Hits          706      713       +7     
- Misses        144      173      +29     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"dist/components/js.vue",
"dist/components/js.vue.d.ts",
"dist/components/js.vue.d.mts",
Copy link
Contributor

@Teages Teages Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declaration file of vue is special, perhaps it should be .vue.d.ts?

I did some test and it looks like typescript cannot read the types from *.vue.d.mts when importing *.vue

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but it looks like it should actually be .d.vue.ts https://www.typescriptlang.org/tsconfig/#allowArbitraryExtensions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaration for vue has a longer history than this flag, and the most of vue community is currently using .vue.d.ts.

I think it would be fine that just skip vue declaration. So that we can automatic switch to .vue.d.ts if the community decides to switch because we are also using vue-tsc to generate vue declaration

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I completely agree. I'll make sure those files stay untouched as part of this change and follow up with a separate issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants