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.
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 |
patak-dev
added
the
p3-minor-bug
|
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. |
Niputi
changed the title
|
@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
.jsextensions for all formats, but this is not Node compliant. It should emit.mjsfor es formats and.jsfor cjs formats with"type": "commonjs"and.jsfor es formats and.cjsfor cjs formats with"type": "module".Additional context
The current default
esextension 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).