-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: support rollup plugin this.load in plugin container context #11469
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.
Thank you for the PR!
// Don't throw an error when returning from an async function | ||
if (key === 'then') { | ||
return undefined | ||
} |
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.
TIL about this 😮
const p = new Proxy({}, { get() { throw new Error('foo') } })
async function foo() { return p }
;(async () => {
await foo() // error
})()
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.
Yeah that threw me for a loop initially - I believe this is due to async/await being syntactic sugar for a promise, and promises being able to return promises, so the runtime needs to "probe" for a .then()
on the return to see whether it needs to chain the promise.
Co-authored-by: 翠 / green <green@sapphi.red>
Sorry for attending this 2 months late 😬 Let's get this in 4.2 |
…tejs#11469) Co-authored-by: 翠 / green <green@sapphi.red>
Description
This adds support for calling this.load (added in Rollup 2.60) in a rollup plugin when using a plugin context via Vite's plugin container (namely, when using the dev server).
Resolves #6810
Additional context
This fixes the issue I have in my specific use case (rollup-plugin-typescript2 needing to call this.load, though not using any of its output - I tested this PR locally with that test case), and is strictly more functional than the previous lack of support. However, there are some limitations which I don't know if it's appropriate to address or otherwise present issues:
this.load
from Vite will be a ModuleInfoProxy, which doesn't include most of the properties from ModuleInfo. I imagine some plugins are likely to be callingthis.load
expecting some of these parameters to work. That said, this is already an existing limitation of getModuleInfo, so I assume that is out of scope.this.load
is supposed to triggertransform
andmoduleParsed
- this does not (in accordance with vite's on-demand nature)I don't have much experience working at this level, so feedback is definitely appreciated, and I'm happy to make any adjustments
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).