-
-
Notifications
You must be signed in to change notification settings - Fork 440
fix(rolldown-compat): Only apply footer to content and unlisted scripts via plugin #1793
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(rolldown-compat): Only apply footer to content and unlisted scripts via plugin #1793
Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
A related issue: #1738. The script does not need to set a global variable at all for the return value from the IIFE to be returned by executeScript. |
If i'm understand you right, this plugin is unnecessary at all, yeah? I can simply remove all of that, include esbuild prop from original solution, yes? |
|
We should probably get input from the project authors on this. |
sm17p
left a comment
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.
Nice work!
| generateBundle(_, bundle) { | ||
| for (const chunk of Object.values(bundle)) { | ||
| if (chunk.type === 'chunk' && chunk.isEntry) { | ||
| chunk.code += `${iifeReturnValueName};`; |
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.
This works, but linking relevant issues for support at bundler level
- top level return for IIFEs rollup/rollup#5509 (comment)
-
this.parse now only supports the allowReturnOutsideFunction option for now ([v4.0] Switch parser to SWC and introduce native/WASM code rollup/rollup#5073)
- feat!: rollup v4 vitejs/vite#14508
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.
@sm17p I don't understand.
What are you want to achive, i need to adjust my code or what?
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.
What I found is that there is a rollup specific option called allowReturnOutsideFunction which does what this plugin is trying to do.
The first issue has usage examples for areas where we need that option for code transformation.
The 2nd and 3rd are just supporting issues indicating that it's available for use for Vite v6.
Why I mentioned it?
- I thought if it's available at a bundler level, then it's better to use that to avoid maintenance overhead.
i need to adjust my code or what?
- Upto the owner's descretion
- Normally, what I would do is check if I can get it working with already existing API but that's your call
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.
@sm17p Now i see, what are you want to achive.
But this prop could be use only inside plugin context, i need to make plugin after all, especially if we want to run it under condition(Only for content-script and unlisted-script)
I was studying the structure of project and of building process, and there's no place do enable it for plugin of one of entrypoints, because it's fetching dynamically, and there's no plugin for e.g content-script in which plugin i can enable allowReturnOutsideFunction.
Maybe there's a chance. owner will answer, i'm still begginer in this project :)
Now it's simply IMO, but if it could be more simply, let's feel free to open PR to my branch with your solution or wait for owner answer, we knows more than us, about his product :)
aklinker1
left a comment
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.
Thanks!
@wxt-dev/analytics
@wxt-dev/auto-icons
@wxt-dev/browser
@wxt-dev/i18n
@wxt-dev/module-react
@wxt-dev/module-solid
@wxt-dev/module-svelte
@wxt-dev/module-vue
@wxt-dev/runner
@wxt-dev/storage
@wxt-dev/unocss
@wxt-dev/webextension-polyfill
wxt
commit: |
|
Thanks for helping make WXT better! |
Overview
I hope that's it :)
Manual Testing
Let's run
pnpm -F wxt-demo devand then check ifwxt-demo/.output/chrome-mv3-dev/content-scripts/automount.jshaveautomounton the very last line.Related Issue
This PR closes #1717