-
Notifications
You must be signed in to change notification settings - Fork 114
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: esbuild support for addWatchFile and getWatchFiles #345
Conversation
LGTM, @antfu could plz you review together? |
@@ -48,7 +48,7 @@ export interface UnpluginOptions { | |||
buildEnd?: (this: UnpluginBuildContext) => Promise<void> | void | |||
transform?: (this: UnpluginBuildContext & UnpluginContext, code: string, id: string) => Thenable<TransformResult> | |||
load?: (this: UnpluginBuildContext & UnpluginContext, id: string) => Thenable<TransformResult> | |||
resolveId?: (id: string, importer: string | undefined, options: { isEntry: boolean }) => Thenable<string | ExternalIdResult | null | undefined> | |||
resolveId?: (this: UnpluginBuildContext & UnpluginContext, id: string, importer: string | undefined, options: { isEntry: boolean }) => Thenable<string | ExternalIdResult | null | 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.
Adding this would indicate that it works for every bundler. Would love to have a simple test for it.
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 added a simple test for this in resolveId
(and perhaps we should in transform
and load
too).
And good that we tested: Webpack doesn't do this. Does anyone know whether Resolver
s in Webpack can access a compilation
object, so we can call createContext
? I didn't see an easy way... One option would be to throw an error in these situations.
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've done a bunch of searching and it seems like loaders and resolvers are treated very differently in Webpack, and only loaders have all the APIs we need. I'm unclear whether a loader can be used as a resolver. But at least for now, I've added the same consistent interface to resolveId
(so types work and function calls don't crash from lack of functions), but most of the API functions throw
errors. (Some do work; I could implement error
, warn
, and parse
still.)
Also documented the limitations here (which are fewer than before β previously resolveId
offered no this
API in Webpack or esbuild).
@antfu This should be ready for re-review when you get a chance. Thanks! |
Any chance this could be merged and released? It's a bottleneck for Civet now, and I'd prefer not to depend on a fork. Alternatively, I could remove the changes to |
Thank you!! |
π Linked issue
#53 (for esbuild)
β Type of change
π Description
This PR adds
this.addWatchFile
support for esbuild in the context of hooksresolveId
,load
, andtransform
.this.warn
andthis.error
were already implemented, which is to build an array for the hook context and then return it from the esbuild hook.resolveId
, I had to.call
resolveId
withthis
set to the actual context, whereas previouslyresolveId
was called withthis
set to the options object. I believe the new behavior aligns with the Rollup spec.createEsbuildPluginContext
helper because we needed it for both resolve and build.I also added a basic
this.getWatchFiles()
which returns the current array of watched files added viathis.addWatchFile
.Context:
this.addWatchFile
for a transpiling unplugin becauseresolveId
returns a not-real filename (with added.tsx
extension to force the output to behave like TypeScript)."file"
, and unplugin sets the context to the plugin name, soesbuild --watch
currently doesn't work at all.I also removed some accidental (I think) modification of the
context
object viaObject.assign
.I have tested this in the context of our unplugin, but I'm not sure how to write tests for this within the current testing infrastructure.
π Checklist