Skip to content
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(optimizer): replace user defines #9548

Closed
wants to merge 2 commits into from
Closed

feat(optimizer): replace user defines #9548

wants to merge 2 commits into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 5, 2022

Description

Fix #9527
Ref #8606 (comment)

Additional context

@Chen-jj I wonder if your issue is somewhat similar to #6007 too, which I have a fix for it, but nonetheless I think this change is good too so we can leverage lesser filesystem traversing (and other optimizations)


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.

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 5, 2022
@Chen-jj
Copy link
Contributor

Chen-jj commented Aug 5, 2022

@Chen-jj I wonder if your issue is somewhat similar to #6007 too, which I have a fix for it, but nonetheless I think this change is good too so we can leverage lesser filesystem traversing (and other optimizations)

I think the situations are same, both would break esbuild's prebundle when dynamic importing optional dependencies.The difference is i use a defined global as condition, when the others use import.meta.env as condition which __vite-optional-peer-dep prefix can help.

@patak-dev
Copy link
Member

@bluwy #8606 fixed a real issue by removing the user defines, see #8606 (comment). Maybe what we could do is only eagerly define when pre-bundling the user defines that are not objects. The bug only appeared for an object value, and for the use cases like @Chen-jj ones it would be quite rare if it isn't a string or simple value.

@bluwy
Copy link
Member Author

bluwy commented Aug 9, 2022

I forgot about prebundling in build aspect 🤦 I wonder if it's also worth trying the second commit of #8606. It should be better for perf too, the cons are negligible i think.

@bluwy
Copy link
Member Author

bluwy commented Aug 9, 2022

I'll close this at the meantime. I think either ways #9321 should fix this implicitly, and we can look into the perf optimizations at a later time.

@bluwy bluwy closed this Aug 9, 2022
@bluwy bluwy deleted the optimizer-define branch August 9, 2022 10:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-add esbuild's define option
3 participants