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

fix(lib): use proper extension #6827

Merged
merged 10 commits into from
May 5, 2022
Merged

Conversation

sachinraja
Copy link
Contributor

@sachinraja sachinraja commented Feb 9, 2022

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@@ -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'
Copy link
Contributor Author

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

@bluwy
Copy link
Member

bluwy commented Feb 9, 2022

Nice! Just noticed this quirk in the docs today as well. For detecting extensions, I think we should base on type: module from package.json instead, and choose the correct extension between .mjs, .js, and .cjs.

@sachinraja
Copy link
Contributor Author

For detecting extensions, I think we should base on type: module from package.json instead, and choose the correct extension between .mjs, .js, and .cjs.

Good idea, just implemented that. I'll add a few tests for it later.

@patak-dev patak-dev added this to the 2.9 milestone Feb 9, 2022
@patak-dev
Copy link
Member

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.

@bluwy
Copy link
Member

bluwy commented Feb 11, 2022

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.

@sachinraja
Copy link
Contributor Author

sachinraja commented Feb 11, 2022

I'm not sure this will actually affect a lot of people considering the docs recommended using a custom fileName function, but I would also say this is a breaking change.

@sachinraja
Copy link
Contributor Author

sachinraja commented Feb 12, 2022

Could this be released under a config option like emitNodeCompliantExtensions? I think it'd be good to get this fix out as soon as possible, there are already too many packages that don't use the correct extensions.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 12, 2022
@bluwy
Copy link
Member

bluwy commented Feb 13, 2022

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 Niputi changed the title fix: use proper extension in library mode fix(lib): use proper extension Feb 22, 2022
@Niputi Niputi mentioned this pull request Feb 22, 2022
9 tasks
@patak-dev patak-dev modified the milestones: 2.9, 3.0 Feb 25, 2022
@patak-dev
Copy link
Member

@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 es. and cjs. prefixes are no longer needed if we use .mjs and .cjs

@sachinraja
Copy link
Contributor Author

Sounds good. I think removing the format prefixes for esm and cjs and would be good, if everyone agrees.

@mangs
Copy link

mangs commented Apr 27, 2022

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 (<script type="module">)?

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 .mjs files. Then again, the popularity of this project could push the ecosystem to evolve to support it; I'm open to either. What are your thoughts? I'm sure I'm not alone in this.

And of course thank you for making such a great tool. 😀

@sachinraja
Copy link
Contributor Author

@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.

@mangs
Copy link

mangs commented Apr 29, 2022

OK I will, thank you for the feedback

@mangs
Copy link

mangs commented Apr 29, 2022

Filed an issue here: #7963

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: library mode p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants