-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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(lib): use proper extension #6827
Conversation
packages/vite/src/node/build.ts
Outdated
@@ -681,7 +681,8 @@ export function resolveLibFilename( | |||
'Name in package.json is required if option "build.lib.fileName" is not provided.' | |||
) | |||
|
|||
return `${name}.${format}.js` | |||
const extension = format === 'es' ? 'mjs' : 'js' |
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.
This only checks for es
format now, but it could check if the format includes es
to support #6555
Nice! Just noticed this quirk in the docs today as well. For detecting extensions, I think we should base on |
Good idea, just implemented that. I'll add a few tests for it later. |
LGTM @sachinraja! Just to be safe, let's merge this one during the 2.9 beta so we give the chance to ecosystem players to test it. I don't think this should be an issue, but changing the extension of the lib output feels too big for a patch. |
Come to think of it, I think this is a breaking change and should be released in a major. If existing libraries rely on the default output path and upgraded Vite, the publishing would be incorrect. |
I'm not sure this will actually affect a lot of people considering the docs recommended using a custom |
Could this be released under a config option like |
I don't think it's necessary to add a new option for this. But at the meantime, we could make a separate PR to update the docs regarding using proper extensions too. |
@sachinraja we talked about this PR with the team, and we would like to merge it but we should wait until Vite 3.0 because of the breaking change. In the meantime, we need to review the naming because the |
Sounds good. I think removing the format prefixes for esm and cjs and would be good, if everyone agrees. |
This is great news! I've been using a custom Rollup config to workaround this for a frontend project until now, so I'm curious can this be exposed for browser-based projects too ( I prefer it for clarity purposes and it allows for easier use in isomorphic projects (Node.js and in the browser), but there is apparently the downside of MIME type support by legacy web servers (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#aside_%E2%80%94_.mjs_versus_.js). I'd love this to be the default behavior, but it may be a bad choice to do so for those unaware of the potential broken rendering due to missing MIME type for And of course thank you for making such a great tool. 😀 |
@mangs I'm not sure if they've discussed that already but I think it would be better to create a separate issue for that. |
OK I will, thank you for the feedback |
Filed an issue here: #7963 |
Description
Library mode currently emits
.js
extensions for all formats, but this is not Node compliant. It should emit.mjs
for es formats and.js
for cjs formats with"type": "commonjs"
and.js
for es formats and.cjs
for cjs formats with"type": "module"
.Additional context
The current default
es
extension will break in Vitest and will need to be inlined.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).