-
-
Notifications
You must be signed in to change notification settings - Fork 218
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: experimentalDts
for Node16
/NodeNext
module resolution
#1225
base: main
Are you sure you want to change the base?
fix: experimentalDts
for Node16
/NodeNext
module resolution
#1225
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
0523610
to
dd055c8
Compare
dd055c8
to
bf631ea
Compare
bf631ea
to
71cdb08
Compare
cfd14d3
to
ba5add4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To matches the behavior of tsc
's output, .js
should only be added for Node16
/NodeNext
, not for Bundler
etc.
@ocavue |
ba5add4
to
5ebadf5
Compare
Thanks. TIL. Does other |
@ocavue I think so, but just to be safe let me double check. |
5ebadf5
to
0d8bd49
Compare
@ocavue Answer is yes! |
I still believe it’s safer to match In my opinion, a better approach would be to use |
@ocavue Can't say I agree with that but if we wanted to offer some more flexibility we could put this functionality behind an option or at least provide an option to disable it? Something like |
|
@ocavue What do you think of removing
I was going for something that was explicit and self-explanatory. While it may sound complicated, it sounds fairly obvious what it does. But fair enough, what would you have in mind? |
Most packages only have one entrypoint, and removing I just don't think it's necessary to config |
012c3d2
to
b22d80f
Compare
b22d80f
to
95b52c5
Compare
@ocavue I tried recreating the issue that you said this would cause, but I can't. I'm going to try some more, but is there anyway you could provide a more concrete example so I can make sure this PR doesn't cause the issues you're describing? Edit: I added some tests for it, please let me know if it needs anything else. |
4b6d4be
to
264980b
Compare
ddec6b6
to
ba6eb7d
Compare
Here you are:
|
@ocavue ahh I see, this is mostly due to how TypeScript handles git clone -b tsup-dts-unique-symbols https://github.com/aryaemami59/tsup-multi-dts |
ba6eb7d
to
59e7805
Compare
import { defineConfig } from "tsup";
export default defineConfig([
{
entry: ["src/foo.ts", "src/bar.ts"],
format: ["esm"],
dts: true,
},
]); |
This PR:
experimentalDts
to work correctly under TypeScript'sNode16
andNodeNext
module resolution.experimentalDts
does not work withNode16
andNodeNext
module resolution #1224.