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

fix: add metadata to jsx when using @vite/plugin-react-refresh #1933

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Feb 7, 2021

This PR is derived from the work at https://github.com/threepointone/vite-plugin-react-jsx. It has 2 specific changes:

  • It adds enforce: 'pre' to the plugin, so that it gets called before it gets passed through esbuild (i.e. before it loses jsx tags). It modifies the parserOpts passed to babel to account for this.
  • It adds 2 babel plugins (transform-react-jsx-self, transform-react-jsx-source) which adds metadata on to jsx tags, allowing tools like React Devtools to show filename and line number information for an element.

There's a separate conversation to have about providing support for React's new jsx transform, and potentially renaming this plugin (if at all), but we can talk about that later. There's value in just landing this separately right now imo.

@threepointone threepointone changed the title fix: add meta information to jsx when using @vite/plugin-react-refresh fix: add metadata to jsx when using @vite/plugin-react-refresh Feb 7, 2021
@threepointone threepointone force-pushed the react-jsx-enhancements branch 2 times, most recently from 37b7bb3 to 788c457 Compare February 7, 2021 17:45
@threepointone
Copy link
Contributor Author

I might need some help here. I've got the test I added passing locally, but seeing different flaky failures across runs here in CI. Any pointers? Thank you!

@remorses
Copy link
Contributor

remorses commented Feb 7, 2021

Using enforce pre will pass typescript code directly to the Babel react refresh plugin, I am not sure this is supported by the react refresh plugin

@threepointone
Copy link
Contributor Author

That's a good point. I did try it and I thought it worked, maybe I was mistaken. I'll investigate later.

(That's not related to the failing test tho :( )

@threepointone threepointone force-pushed the react-jsx-enhancements branch 2 times, most recently from c1b4b8b to a67c641 Compare February 7, 2021 21:06
@threepointone
Copy link
Contributor Author

I'm a bit confused by why the tests are failing, and would appreciate someone explaining it to me?

  • My added test only fails during test-build.
  • During this phase, shouldSkip in the plugin gets set to true because config.command === 'build' || config.isProduction is true.
  • But then why do the other tests in react.spec.ts pass? Odd.
  • editFile has an early return when isBuild is true. Why then does the hot update even work?

I'm clearly missing some context haha. Could someone explain the spookiness to me? And should I just skip my tests when isBuild === true?

@threepointone threepointone force-pushed the react-jsx-enhancements branch 2 times, most recently from 61e7f9d to 688a23c Compare February 7, 2021 21:21
@yyx990803
Copy link
Member

@threepointone ah yes, to make HMR tests eaiser, editFile and untilUpdated are both noops in build tests 😅

Yes you should just skip your test for build.

This PR is derived from the work at https://github.com/threepointone/vite-plugin-react-jsx. It has 2 specific changes:

- It adds `enforce: 'pre'` to the plugin, so that it gets called before it gets passed through `esbuild` (i.e. before it loses jsx tags). It modifies the parserOpts passed to babel to account for this.
- It adds 2 babel plugins (`transform-react-jsx-self`,  `transform-react-jsx-source`) which adds metadata on to jsx tags, allowing tools like React Devtools to show filename and line number information for an element.

There's a separate conversation to have about providing support for React's new jsx transform, and potentially renaming the plugin (if at all), but we can talk about that later. There's value in just landing this separately right now imo.
@threepointone threepointone force-pushed the react-jsx-enhancements branch from 688a23c to 324eade Compare February 8, 2021 14:58
@threepointone
Copy link
Contributor Author

Aha, I missed that untilUpdated was a no-op too 😅. Updated, tests pass woooo

@yyx990803 yyx990803 merged commit 4037c55 into vitejs:main Feb 8, 2021
@threepointone threepointone deleted the react-jsx-enhancements branch February 8, 2021 15:30
@michael-land
Copy link

I'm getting this error after upgrade to 1.3, any ideas how to solve it?

[vite] Internal server error: ...path_to_fold/packages/...path_to_package/src/RichTextEditor/RichTextEditor.tsx: Identifier 'RichTextEditor' has already been declared (117:16)

@threepointone
Copy link
Contributor Author

Could you file a new issue with more details and a minimal repro?

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.

4 participants