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!: transform jsx with esbuild instead of babel #9590

Merged
merged 14 commits into from
Nov 8, 2022

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented Aug 9, 2022

Description

Resolves #9429

Additional context

Development JSX transforms

esbuild supports dev-only JSX transforms (i.e. jsxDev: true) but this only works when using the automatic runtime. To preserve this functionality with the classic runtime, @babel/plugin-transform-react-jsx-self and @babel/plugin-transform-react-jsx-source will still be used in development when using the classic runtime.

Automatic runtime in pre-compiled dependencies

This PR removes the existing restoreJsx functionality (intended to allow dependencies to use the automatic runtime) because 1) it already isn't working in the majority of cases and 2) conflicts with the use of esbuild to compile JSX. A detailed summary of the context around this was written by @cyco130 #9590 (comment)


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.

@patak-dev
Copy link
Member

Thanks for the PR @rtsao! The change looks good to me.

About jsxPure, I think we could remove the option and force it to be always true for runtime classic.

@aleclarson @frandiox @cyco130 would you check how this PR works in your projects? I think we could release it in a new major for plugin-react.

@cyco130
Copy link
Contributor

cyco130 commented Aug 15, 2022

This doesn't work right now: It results in syntax errors because it leaves JSX untransformed in some cases. It seems to be because of the restoreJsx plugin: The Babel JSX plugin used to come after it, but the ESBuild transform seems to come before. ESBuild does JSX => JS and (in some cases) restoreJsx does JS => JSX

I'll look into it a bit more and try to create a reproduction but I don't see a quick fix (apart from removing the restoreJsx plugin, which works).

@cyco130
Copy link
Contributor

cyco130 commented Aug 15, 2022

Turns out it requires an uncommon combination: a React component in node_modules, excluded from optimizeDeps. I've been looking but I haven't been able to find a React component package that works when excluded from optimizeDeps (most of them choke on the prop-types import).

restoreJsx doesn't kick in for optimized deps because the optimized code is too garbled for it to detect React.createElement calls with a regexp. So in the common case it wasn't doing its job anyway. Maybe we should just remove it along with this PR?

@cyco130
Copy link
Contributor

cyco130 commented Aug 15, 2022

TLDR: We have two problems. 1. restoreJsx is not working most of the time because it can't parse optimized deps. 2. restoreJsx breaks this PR under rare circumstances. It would break more often if problem 1 didn't exist.

Repro tomorrow.

@Dunqing
Copy link
Contributor

Dunqing commented Aug 17, 2022

I have created a vite plugin for this purpose.

I made some changes, removed restoreJSX and adjusted the execution order, now esbuild->react plugin.

Now it can work normally at present also a lot faster

@yann-combarnous
Copy link

I have created a vite plugin for this purpose.

I made some changes, removed restoreJSX and adjusted the execution order, now esbuild->react plugin.

Now it can work normally at present also a lot faster

I confirm, using this plugin, dev server does feels faster. Will try and put comparison numbers tomorrow.

@yann-combarnous

This comment was marked as spam.

* Toggles whether or not to throw an error if an XML namespaced tag name is used.
* @default true
*/
jsxThrowIfNamespace?: boolean
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 saw this option was added in #9571

This is not a breaking change as esbuild supports JSX namespaces: evanw/esbuild@71240d4

So Vite would not throw transforming JSX namespaces and thus this option is no longer needed

@sapphi-red sapphi-red added plugin: react p3-significant High priority enhancement (priority) and removed needs rebase labels Oct 25, 2022
@rtsao rtsao requested review from aleclarson and patak-dev and removed request for aleclarson and patak-dev November 7, 2022 21:32
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks for your work and patience with this PR @rtsao. Let's merge it and release an alpha for the next version of plugin react, so we can test it and release it together with Vite 4. Also taking into account here the previous review from @aleclarson.

If you are interested in getting more involved with this plugin @rtsao, reach out to us at https://chat.vitejs.dev in the #contributing channel. For context, we are planning to move this plugin (and plugin-vue and plugin-vue-jsx) out of the Vite core monorepo. We think that using vite-ecosystem-ci we can get a proper testing feedback loop, and the plugin being in a separate repo will help us avoid coupling and make it easier for other contributors to take ownership.

@patak-dev patak-dev changed the title feat: transform jsx with esbuild instead of babel feat!: transform jsx with esbuild instead of babel Nov 8, 2022
@patak-dev patak-dev merged commit f677b62 into vitejs:main Nov 8, 2022
@o-alexandrov
Copy link

It would be interesting to know your thoughts on the overall performance-related direction (given this ticket is about performance) in this discussion

@patak-dev
Copy link
Member

@o-alexandrov answered there. We're going to provide guidance to use the current plugin for React or an SWC-based version when we release Vite 4. By using SWC, you are trading off flexibility (not being able to use babel plugins) and package size (~3x bigger) for HMR and cold start speed. The best option depends on the size of your project, and what libraries and patterns your project is using.

@silverwind
Copy link

Is SWC a must for what the plugin needs? HMR with just esbuild is still impossible, right?

@patak-dev
Copy link
Member

@silverwind, yes, it is out of the scope of esbuild at this point: evanw/esbuild#151
We need Babel, or SWC as used by @ArnaudBarre to create https://github.com/ArnaudBarre/vite-plugin-swc-react-refresh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin react] use esbuild's automatic JSX transform instead of babel