-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
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?