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: entrypoint specified in module #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tasshi-me
Copy link

This PR fixes #155.
I made the entrypoint specified in the module field the same as exports.

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I haven't been able to reproduce the reported problem due to vite changes, but looked at the documentation and files and agree this change mades the (legacy) module field as intended.

@shadowspawn
Copy link
Member

As a sanity check I looked at what some of the other Yargs libraries have, and import entry point and "module" do match each other.

https://github.com/yargs/yargs-parser/blob/3aba24ceaa1a06ceb982c63a06002526d781e826/package.json#L6-L19

https://github.com/yargs/cliui/blob/af3145da0ea31738c4715865a6da0ee388a94c74/package.json#L6-L16

@shadowspawn
Copy link
Member

I note you put fix! for a breaking change in the title. Do you feel it is a breaking change, or is it just a bug fix?

(I don't have much experience with bundlers and interested in your reasoning.)

@tasshi-me
Copy link
Author

@shadowspawn
Thank you for reviewing! You can remove !.

I added it because this PR may break environments using older versions of bundlers that don't support the exports field.
I don't know how many users use this package directly and are impacted.

I checked when webpack and rollup started supporting exports.

@shadowspawn
Copy link
Member

Thanks for info, and good links.

I found a couple of links covering background and usage of the "module" main field:

@tasshi-me
Copy link
Author

@shadowspawn

Thank you for letting me know!

  • I understand the module field is not official, but major bundlers supported it in old versions.
  • Also, I found y18n has 1,048 dependents.
  • The only action the users should take is to change the named import to the default import

Now, my idea...

  1. Release with fix, because it changes the interface for old bundlers, but the user impact is small
  2. Release with fix!, even if user impact is small
    1. I think it's nice to drop Node 10 and 12 on the same major update.

I respect your decision.

@tasshi-me tasshi-me changed the title fix!: entrypoint specified in module fix: entrypoint specified in module Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The files specified in module and exports are mismatched.
2 participants