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): add user esbuild plugins support #2886

Closed
wants to merge 1 commit into from

Conversation

qnp
Copy link
Contributor

@qnp qnp commented Apr 6, 2021

Description

Quoting esbuild-plugin-alias:

Sometimes it's useful to have dynamic imports that resolves into different files depending on some conditions (e.g. env variables).

In my case, I had to shim all internal Vuetify VIcon imports to replace it with a custom and improved component ZIcon. With vue-cli-3 and Webpack, all was fine. With Vite, I use this config:

// vite.config.js
  resolve: {
    alias: {
      '../VIcon': path.resolve(__dirname, 'src/components/ZIcon'),
      '../../VIcon': path.resolve(__dirname, 'src/components/ZIcon'),
    },
  },

Aliases are properly handled by rollup at compilation. However, they are not matched by esbuild (the Vuetify component is imported).
I thought about adding internally to Vite an import to esbuild-plugin-alias and plug to it the config aliases, but not sure about side effects and the versatility of this solution – I guess it is an edge case, and more profitable to all if esbuid’s plugin system is exposed.

Hence, I propose to add an entry esbuildPlugins to Vite config as such:

// vite.config.js
import alias from 'esbuild-plugin-alias';
export default {
  ...
  esbuildPlugins: [
    alias({
      '../VIcon': path.resolve(__dirname, 'src/components/ZIcon/index.js'),
      '../../VIcon': path.resolve(__dirname, 'src/components/ZIcon/index.js'),
    })
  ],
}

Additional context

Do you think it is the correct way to tackle the above mentioned issue ?

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p2-edge-case Bug, but has workaround or limited in scope (priority) label Apr 7, 2021
Comment on lines +113 to +116
/**
* Array of esbuild plugins to use.
*/
esbuildPlugins?: EsbuildPlugin[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: IMO this should be part of ESBuildOptions and not its own property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, however it may introduce breaking changes, as Vite config esbuild option (ESBuildOptions) are passed directly to esbuild transform function, not to build function. See:

We could think about:

  1. Split the actual esbuild config into:
esbuild: {
   transform: { /* the current `esbuild` options */ },
   build: { plugins: [/* the esbuild plugins */] },
},

but this is a breaking change

  1. Keep the actual model and add plugins to it, or build.plugins to be clearer and allow for more flexibility in future developments:
esbuild: {
   /* actual options */,
   build: { plugins: [/* the esbuild plugins */] },
},

and do delete resolvedOptions.build in transformWithEsbuild function before passing the options.

I am not convinced about the second solution.

I think the first one is much more clear, and we could go a step further by:

  1. using the root of ebuild options (as today) for include, exclude and jsxInject options (which are deleted from options before the latter object is passed to esbuild.transform – so doing so we avoid this deletion and directly pass options.transform to esbuild.transform(), while options.build are passed directly to esbuild.build() with an appropriate merge):
esbuild: {
   include: ...,
   exclude: ...,
   jsxInject: `import React from 'react'`,
   transform: {
     jsxFactory: 'h',
     jsxFragment: 'Fragment',
   },
   build: {
     plugins: [/* the esbuild plugins */],
     /* other future options */
   },
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 I really like your proposal 👍

@patak-js (@antfu) What are your thoughts about this?

Also IMO a breaking change shouldn't hold us back for such an improvement because it may be more future-proof

Copy link
Contributor Author

@qnp qnp Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, if @patak-js and @antfu validate, I'll do the coding.
What is the roadmap for such a feature to be included in next minor ?
I'm pretty much impatient to use it ;-)

Copy link
Member

@nihalgonsalves nihalgonsalves Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like since optimizeDeps is an entirely different build phase than the esbuild transform options, the settings should go into config.optimizeDeps. Here's a demonstration of that, perhaps this helps: #2991

I'd really like to get this functionality soon, since we're blocked by this in our conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants