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

feat: esbuild support for addWatchFile and getWatchFiles #345

Merged
merged 13 commits into from
Dec 26, 2023

Conversation

edemaine
Copy link
Contributor

πŸ”— Linked issue

#53 (for esbuild)

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR adds this.addWatchFile support for esbuild in the context of hooks resolveId, load, and transform.

  • The implementation is similar to how this.warn and this.error were already implemented, which is to build an array for the hook context and then return it from the esbuild hook.
  • To support resolveId, I had to .call resolveId with this set to the actual context, whereas previously resolveId was called with this set to the options object. I believe the new behavior aligns with the Rollup spec.
  • I factored out a 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 via this.addWatchFile.

  • My understanding is that this doesn't match the Rollup spec of returning all watched files, including implicitly watched files. I've added a note to the README accordingly.
  • But this behavior seems better than nothing / the current behavior of returning the empty array.
  • I could also remove it if this seems too partial an implementation.

Context:

I also removed some accidental (I think) modification of the context object via Object.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

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

src/esbuild/utils.ts Outdated Show resolved Hide resolved
@sxzz sxzz requested a review from antfu October 16, 2023 14:03
@sxzz
Copy link
Collaborator

sxzz commented Oct 17, 2023

LGTM, @antfu could plz you review together?

README.md Outdated Show resolved Hide resolved
@@ -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>
Copy link
Member

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.

Copy link
Contributor Author

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 Resolvers 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.

Copy link
Contributor Author

@edemaine edemaine Oct 20, 2023

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).

src/esbuild/utils.ts Outdated Show resolved Hide resolved
@edemaine
Copy link
Contributor Author

@antfu This should be ready for re-review when you get a chance. Thanks!

@edemaine edemaine mentioned this pull request Nov 5, 2023
5 tasks
@edemaine
Copy link
Contributor Author

edemaine commented Dec 5, 2023

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 resolveId if they're too controversial, and just keep the changes for esbuild in load, which is what we really need (because this is the only place addWatchFile is supported across all build systems.

@sxzz sxzz requested a review from antfu December 14, 2023 16:34
@antfu antfu merged commit 2f65939 into unjs:main Dec 26, 2023
4 of 7 checks passed
@edemaine
Copy link
Contributor Author

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants