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

chore: emit .d.cts files #10945

Closed
wants to merge 2 commits into from
Closed

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Nov 15, 2022

Description

This will allow CJS projects using moduleResolution: node16 to import Vite.

It copies the types from dist/node into dist/node-cjs, changing file extension from .d.ts to .d.cts and rewriting relative imports to include .cjs suffix.

It only does this on pnpm build.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@aleclarson aleclarson added the p4-important Violate documented behavior or significantly improves performance (priority) label Nov 15, 2022
@aleclarson aleclarson marked this pull request as draft November 15, 2022 19:51
@aleclarson aleclarson force-pushed the chore/emit-cjs-types branch 2 times, most recently from c9c53cf to 7fef940 Compare November 15, 2022 19:54
@aleclarson

This comment was marked as outdated.

@aleclarson aleclarson force-pushed the chore/emit-cjs-types branch 2 times, most recently from 47a699f to 3a369fb Compare November 15, 2022 20:20
@aleclarson aleclarson marked this pull request as ready for review November 15, 2022 20:21
@aleclarson
Copy link
Member Author

I think we'll want to run this script in pnpm dev too. But how? 🤔

name: 'cjs-types',
async generateBundle() {
const promisedSpawn = promisify(spawn)
await promisedSpawn('pnpm', ['build-emit-cjs-types'], {
Copy link
Member Author

Choose a reason for hiding this comment

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

So I'm spawning pnpm build-emit-cjs-types in development mode 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Works like a charm ✨

@aleclarson aleclarson requested a review from patak-dev November 15, 2022 21:42
}

const outFile = file
.replace('/node/', '/node-cjs/')
Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we should relativize file before doing this replacement

This will allow CJS projects using "moduleResolution: node16" to import Vite.
plugins: [
...createNodePlugins(false, false, false),
!isProduction && cjsTypesPlugin(),
bundleSizeLimit(160)
Copy link
Member Author

Choose a reason for hiding this comment

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

Bumped size limit from 120 to 160

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I'm not a fan of another new file for CJS only, especially that our CJS API already doesn't have full coverage, and we're pushing towards ESM. Maybe it would be fine to use patch-package if they really want to use in CJS?

Comment on lines +33 to +34
const cjsModuleSpec = id + '.cjs'
text = text.slice(0, i.s) + cjsModuleSpec + text.slice(i.e)
Copy link
Member

Choose a reason for hiding this comment

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

So this adds .cjs to all file imports, do we need to do the same for node/index.d.ts to with .js only. I've seen this need in the past with node16, but i've also heard that it's not exactly needed.

I also wonder if adding .js is enough to get CJS support? so we avoid another file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested explicit .js in node/index.d.ts before taking this route. It doesn't work, unfortunately.

@bluwy bluwy added p2-to-be-discussed Enhancement under consideration (priority) and removed p4-important Violate documented behavior or significantly improves performance (priority) labels Dec 29, 2022
@patak-dev
Copy link
Member

Is this still needed with moduleResolution: 'bundler'?

Even then, I also think that the added complexity is too high, and we should start pushing a bit more toward ESM. My vote goes to avoid the extra files here.

@patak-dev
Copy link
Member

Thanks for the PR Alec. We discussed this PR in today's team meeting and we decided it is better to keep the types as is.

We're thinking we should start to push folks to use bundler instead of node16 in general. We're going to start closing some node16 issues in Vite in the future if they are solved with bundler. For this PR in particular, even if importing Vite in CJS ends up not working well with resolution bundler, we think that the added complexity is too much to justify this use case. A bit of friction here won't hurt the ecosystem to keep moving toward ESM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants