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(config): don't resolve by exports.module field #10442

Closed
wants to merge 3 commits into from

Conversation

Dunqing
Copy link
Contributor

@Dunqing Dunqing commented Oct 12, 2022

Description

fix: #10430
related: #10347

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.

@benmccann
Copy link
Collaborator

If this were going to be changed, you would need to update the docs: https://vitejs.dev/config/shared-options.html#resolve-conditions

It looks like this was originally added to support tslib: 3a3029e. That change didn't have a test and I'm not that familiar with why it was added, so I'd want to make sure that removing it isn't breaking anything

I don't understand the motivation for this change or why jotai is setup the way it is. I've asked some questions to clarify in that issue: pmndrs/jotai#1475 (comment)

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 13, 2022

If this were going to be changed, you would need to update the docs: https://vitejs.dev/config/shared-options.html#resolve-conditions

Thank you, updated.

It looks like this was originally added to support tslib: 3a3029e. That change didn't have a test and I'm not that familiar with why it was added, so I'd want to make sure that removing it isn't breaking anything

Yeah, It seems to me that there is no need to support exports.module.

@benmccann
Copy link
Collaborator

Yeah, It seems to me that there is no need to support exports.module.

I'm not sure how you came to that conclusion. I see tslib still has an exports.module field:

Screenshot from 2022-10-14 11-02-09

I think probably no project should be using exports.module as it's a completely non-standard thing. However, it seems to me that if we were to remove it we'd likely break a lot of existing projects since tslib is so widely used. If you want to remove it here, you should fix tslib first: microsoft/tslib#183

I think it should also be removed from jotai. If I have to choose between jotai being broken or tslib being broken, I prefer that jotai is the one that's broken since it has fewer users.

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 15, 2022

I'm not sure how you came to that conclusion. I see tslib still has an exports.module field:

Screenshot from 2022-10-14 11-02-09

Yes, as you can see, tslib also still has exports.import, which is enough for vite to successfully resolve

I think probably no project should be using exports.module as it's a completely non-standard thing. However, it seems to me that if we were to remove it we'd likely break a lot of existing projects since tslib is so widely used. If you want to remove it here, you should fix tslib first: microsoft/tslib#183

I think it should also be removed from jotai. If I have to choose between jotai being broken or tslib being broken, I prefer that jotai is the one that's broken since it has fewer users.

I don't think we need to do anything except remove support for resolve.module. Because we still support the standard exports.

@Dunqing Dunqing force-pushed the fix-10430 branch 2 times, most recently from d2c5aaf to 2388688 Compare October 15, 2022 04:39
@sapphi-red
Copy link
Member

superseded by #10528

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite >=3.1.5 breaks Jotai babel plugins
4 participants