-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: derive proper js extension from package type #8382
Conversation
packages/vite/src/node/build.ts
Outdated
: libOptions | ||
? resolveLibFilename(libOptions, output.format || 'es', config.root) | ||
? resolveLibFilename(libOptions, format, config.root) | ||
: path.posix.join(options.assetsDir, `[name].[hash].js`), | ||
chunkFileNames: libOptions | ||
? `[name].[hash].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.
@sachinraja @antfu shouldn't chunks also have a derived mjs
/cjs
/js
extension for chunks in lib mode (and also SSR?)
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 me I think it's better to consist the extension for both entry and chunks
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.
Yes, they must to be compliant with node resolution
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.
Done, it should now work for SSR and chunks
We discussed with @dominikg that the logic in this PR and in #6827 does not cover the case where there is a I still think this isn't a blocker though. We should still merge this PR as it is an improvement over the current state, and we can discuss these changes (if needed) in a future PR. |
There were no discussions about it unfortunately. IMO it's not necessary but would be nice to have. |
Ok, let's merge this PR for now. Maybe we should wait to get a feature request with a real use case. |
Description
See #8348 (comment)
What is the purpose of this pull request?