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
feat(worker): bundle worker emit asset file #6599
feat(worker): bundle worker emit asset file #6599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks okay to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
We discussed this PR @poyoho in the last team meeting and we can move forward with it. Thanks for keeping improving Vite's Worker story. Let's merge it during the 2.9 beta so we can get some extra testing. |
@ygj6 would you help re-reviewing this PR if you have some time? |
@@ -3,9 +3,10 @@ import { defineConfig } from 'vite' | |||
|
|||
export default defineConfig({ | |||
build: { | |||
target: process.env.NODE_ENV === 'production' ? 'chrome60' : 'esnext' | |||
// target: process.env.NODE_ENV === 'production' ? 'chrome60' : 'esnext' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change related to the esbuild issue @poyoho? Should we wait for it before merging this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I open a issues in vite #7179
and open in esbuild too. evanw/esbuild#2089
I think not. I think chrome60
is not a important unit test.
The config may I copy from the issues repo in last time. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's uncomment this line if the change isn't needed then? Or remove the line directly?
We can merge it afterwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
Description
fix: #6594
fix: #5699
fix: #6706
fix: #7114
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).