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(plugin-legacy): respect modernTargets option if renderLegacyChunks disabled #15789

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

jgosmann
Copy link
Contributor

@jgosmann jgosmann commented Feb 2, 2024

Description

Fixes #15788 .

If modernPolyfills is set to true, but renderLegacyChunks is set to false, genLegacy will be false and we would return early. If this return happens before modernTargets is initialized, the modern polyfills will be generated with the wrong (default?) target instead of the specified modernTargets.

Additional context

#14527 is related and lead to the introduction of modernTargets in #15506.


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Feb 2, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@jgosmann jgosmann changed the title (fix: #15788) Always read targets options fix: always read targets options (fix: #15788) Feb 2, 2024
@jgosmann
Copy link
Contributor Author

jgosmann commented Feb 2, 2024

The cause of the failing build pipeline seems unrelated to the changes to me.

@jgosmann jgosmann marked this pull request as draft February 2, 2024 16:02
@jgosmann
Copy link
Contributor Author

jgosmann commented Feb 2, 2024

Converted this a draft PR because this does not seem to be sufficient to fix the issue.

@jgosmann jgosmann marked this pull request as ready for review February 3, 2024 15:40
@jgosmann
Copy link
Contributor Author

jgosmann commented Feb 3, 2024

This seems to be working now (at least for our use case).

@bluwy bluwy changed the title fix: always read targets options (fix: #15788) fix(plugin-legacy): respect modernTargets option if renderLegacyChunks disabled Feb 6, 2024
@bluwy bluwy added plugin: legacy p3-significant High priority enhancement (priority) labels Feb 6, 2024
If `modernPolyfills` is set to `true`, but `renderLegacyChunks` is set
to false, `genLegacy` will be false and we would return early. If this
return happens before `modernTargets` is initialized, the modern
polyfills will be generated with the wrong (the default?) target instead
of the specified `modernTargets`.
@jgosmann
Copy link
Contributor Author

jgosmann commented Mar 7, 2024

@bluwy I hope you don't mind pinging you. Can we get this merged? Then we would no longer to have to patch this directly in our node_modules. Or is there anything missing?

@bluwy
Copy link
Member

bluwy commented Mar 8, 2024

Sorry I've forgot to follow up on this. No worries on pinging me 👍 This looks good to me. I'll leave this open for awhile if anyone else wants to review this, but otherwise I'll merge and cut a release for this by the end of the day.

@bluwy bluwy merged commit 0813531 into vitejs:main Mar 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority) plugin: legacy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin-legacy: modernTargets not respected when renderLegacyChunks is false
3 participants