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: support esbuild #46

Merged
merged 12 commits into from
Dec 28, 2021
Merged

feat: support esbuild #46

merged 12 commits into from
Dec 28, 2021

Conversation

hyrious
Copy link
Contributor

@hyrious hyrious commented Dec 24, 2021

This pr adds esbuild support for unplugin, which resolves #25 .

Some Todos:

  • What version of esbuild should be set in peerDependencies?
  • Esbuild supports bundling js and css, while we cannot know the module type on load (especially if it is provided by a virtual file). We just assume the results of load and transform are always js variants (js, jsx, ts, tsx) then append the sourcemap url to the code.
  • By esbuild's design the onLoad callback will be applied to all kind of resources, including the binary ones (images, etc.). So the plugin has a chance to handle them too. Plugin authors need to make sure the result of the load and transform are valid javascript code.
  • plugin.load and plugin.transform both returns code and map, we merge the two sourcemaps if both present. Otherwise only the last one will be kept.

Hope to hear your thoughts!

@antfu
Copy link
Member

antfu commented Dec 24, 2021

1: >=0.13 would be better as Vite is still on v0.13
2, 3: I think that should be handled by the plugin authors
4: Would esbuild merge them like rollup/vite are doing? If not, I think we should merge them.

@hyrious
Copy link
Contributor Author

hyrious commented Dec 24, 2021

About 2, 3: plugin authors just returns the sourcemap, without saying what kind of code it is. (Is it js or css?)

code += `\n//# sourceMappingURL=${map.toUrl()}`
// this only work if code is js
// in css we should use `/*#` style

I may assume that plugins only returns javascript. In fact I don't know what other bundlers handle file types.

@hyrious
Copy link
Contributor Author

hyrious commented Dec 24, 2021

About 4: esbuild do merge source maps if the result in onLoad() contains sourcemap url comments, while that's not the case. The case is there's no transform stage in esbuild plugins, thus the implementation just puts plugin.transform in onLoad to match the behavior. We may get two sourcemaps:

const { code, map: map1 } = await plugin.load(id)
const { code: result, map: map2 } = await plugin.transform(code)
return { result, map: ? }

@antfu
Copy link
Member

antfu commented Dec 24, 2021

I think authors should filter out the types that are not js (so they still have the chance to process them if they absolutely want). Could be better to mention that in the docs.

4: then for sure we should merge it

@hyrious
Copy link
Contributor Author

hyrious commented Dec 25, 2021

@antfu I have tested (a little) against your awesome unplugin-icons and depending on the result I adjusted some behavior.

@antfu
Copy link
Member

antfu commented Dec 28, 2021

CI failed, can you have a look?

@hyrious
Copy link
Contributor Author

hyrious commented Dec 28, 2021

This is because loader: default will ask esbuild to choose a loader from the module name, which is virtual/1, so it failed to guess the loader.

Maybe we should guess the loader by ourself.

@antfu
Copy link
Member

antfu commented Dec 28, 2021

Let's have the default js

@hyrious
Copy link
Contributor Author

hyrious commented Dec 28, 2021

A bit more thinking: maybe we should use JSX as default since some users may write jsx in .js files. 🤔

But I'd like to keep it as is until someone come to ask for this feat 😆

@antfu antfu merged commit 28557c8 into unjs:main Dec 28, 2021
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.

Feature: ESBuild compatibility
2 participants