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

Support React __source prop #2035

Closed
4 tasks done
remcohaszing opened this issue May 12, 2022 · 8 comments · Fixed by #2045
Closed
4 tasks done

Support React __source prop #2035

remcohaszing opened this issue May 12, 2022 · 8 comments · Fixed by #2045
Labels
💪 phase/solved Post is done 🦋 type/enhancement This is great to have

Comments

@remcohaszing
Copy link
Member

Initial checklist

Problem

React uses the __source prop display where components originate in the React devtools.

Babel supports this in @babel/babel-plugin-transform-react-jsx-source.

Solution

Inject this prop in a similar way as @babel/babel-plugin-transform-react-jsx-source. This should only happen in development and only for React, but optionally supporting alternative runtimes.

Also I’m willing to implement this.

Alternatives

It could be an external recma plugin, but I think it’s useful to include in core.

@wooorm
Copy link
Member

wooorm commented May 12, 2022

@remcohaszing
Copy link
Member Author

remcohaszing commented May 13, 2022

I found a bit more info in the React RFC for the automatic runtime.

So basically in the classic runtime, we need to compile to:

React.createElement(
  Component,
  {
    ...props,
    __source={{ fileName, lineNumber, columnNumber }}
    __self={this}
  },
  ...children
)

And in the automatic runtime, we need to compile to the following, where isStaticChildren is a boolean indicating if a production runtime would use jsxs instead of jsx:

import { jsxDEV as _jsxDEV } from 'react/jsx-runtime'

_jsxDEV(
  Component,
  props,
  key,
  isStaticChildren,
  { fileName, lineNumber, columnNumber },
  this
)

I still need to figure out when the __self prop or last argument of _jsxDEV needs to be added. It could be added later as well, so I can focus on supporting __source first.

I also believe this shows this functionality belongs in core, especially for the automatic runtime.

@wooorm
Copy link
Member

wooorm commented May 14, 2022

We generate JSX. Because otherwise each step needs to know which runtime the user wants, and sometimes they don‘t want to compile it away.
Much easier to use JSX as the lingua franca, and then at the end optionally transform JSX into function calls.

I think we need to do the same here: add that prop to JSX.

On the receiving end, we can see from Preact and React, that they optionally expect __source. So, if there’s an automatic runtime, we can define __source on JSX. Though estree-util-build-jsx has to move it from props to the 5th argument.

For the classic runtime, as you linked before, as __source is passed in props, and some h functions don’t handle __source meaning it ends up in HTML, we can’t just define and later pass it.

I don’t support _jsxDEV in estree-util-build-jsx yet. Is that required, because otherwise __source is not used in production? We can, if needed, add support for a development option to estree-util-build-jsx.

@remcohaszing
Copy link
Member Author

Maybe we should only focus on the automatic runtime. The main difference is the automatic runtime actually supports a specific dev transform, whereas in the classic runtime some React specific props are used.

estree-util-build-jsx will receive 2 new options, which are only used in development:

options.development

If the automatic runtime is used, this compiles JSX into automatic runtime development mode. (boolean, default: false)

options.file

If the automatic runtime development mode is used, this option is used to provide a file to resolve the JSX node’s file name and position. (VFile)

@mdx-js/mdx will just have to pass through these options. Although maybe there should also be an option to explicitly opt out of this behaviour.

For comparison, TypeScript basically supports 3 types of JSX transforms:

  • react (classic runtime)
  • react-jsx (automatic runtime)
  • react-jsxdev (automatic runtime with dev mode (documented incorrectly))

Babel supports 2 runtime types in their JSX plugin: classic and automatic.

It uses a development option to toggle between the JSX or JSX development plugin in @babel/preset-react.

@wooorm
Copy link
Member

wooorm commented May 16, 2022

options.file

Maybe filePath? I don’t see the rest of file being used, likely. It also helps that file.path does not mean file.history[0] (the original file).

I don’t see a need for an extra @mdx-js/mdx option, to opt out. If we run into trouble, we can add it later.

I’m under the impression that TS is a bit messy with everything it supports for JSX, compared to the comments that we, Babel, and SWC support. I’d focus on modelling Babel!


Sounds like we have a plan?!

@remcohaszing
Copy link
Member Author

I believe we need the file contents, because estree uses offsets which we need to map to lines and columns. Depending on that we need either a vfile or just the file path. I'll figure that out on the go. Apart from that, we have a plan! 👍

@wooorm
Copy link
Member

wooorm commented May 16, 2022

Aren't the positions on nodes enough? 🤔

@remcohaszing
Copy link
Member Author

AST explorer only attaches start and end offsets, but apparently the estree format produced by MDX attaches loc as well. So you’re right, we don’t need any additional information, at least not for MDX.

wooorm pushed a commit to syntax-tree/estree-util-build-jsx that referenced this issue May 18, 2022
Closes GH-2.
Related-to: mdx-js/mdx#2035.

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
wooorm pushed a commit that referenced this issue Dec 14, 2022
The JSX dev runtime exposes more debugging information to users. For
example, React exposes positional information through React Devtools.

Although GH-2035 started off as an issue to support the `__source` prop, I
have refrained from actually supporting the `__source` prop, as it’s
specific to React. The automatic dev runtime supports this information
without patching props.

Closes GH-2035.
Closes GH-2045.

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
@wooorm wooorm added 🦋 type/enhancement This is great to have 💪 phase/solved Post is done labels Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

2 participants