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

Add support for new React 17 JSX transform #585

Closed
alangpierce opened this issue Jan 9, 2021 · 2 comments · Fixed by #740
Closed

Add support for new React 17 JSX transform #585

alangpierce opened this issue Jan 9, 2021 · 2 comments · Fixed by #740

Comments

@alangpierce
Copy link
Owner

alangpierce commented Jan 9, 2021

React has a published a description of a new JSX transform with a corresponding runtime library available in new React versions: https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html . TypeScript (and others) have already implemented the new transform: https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/#jsx-factories . Sucrase should implement some support for it, though the details are still an open question.

Unfortunately, it looks like it's unlikely to simplify the JSX transform for Sucrase, and will likely make it harder. For example:

  • For the prod transform, we'll now need to decide whether to emit jsx or jsxs based on whether the children section (later on in the code) has at least two elements explicitly specified. This sort of "lookahead" behavior isn't well-supported by Sucrase, but could probably be implemented via a special case in the parser and a new token type or a new flag on the token object.
  • We'll need to specially detect the key prop and reorder it to come after the props. Since the key could be an arbitrary expression that may contain newlines, it will be a challenge to correctly preserve line numbers like the rest of Sucrase does. If we simply move the key expression to after the props (preserving newline chars in each), then we'll end up with broken breakpoints if you try to debug within a callback prop. This issue also affected the class fields implementation before I rewrote it in Change class field implementation to use initializer methods #313 to keep the code in order. There might be a clever alternative transform that preserves order, like using a comma expression to assign to an intermediate variable, but it certainly seems like a pain.
  • The transform now requires introducing an import and finding an unused name for each imported identifier rather than the old approach of relying on an existing imported name, which likely means more interdependency between the JSX transform and imports transform (even in the ESM case).
  • The difference between dev and prod modes is now more significant, with a different jsxDEV function that will fail if used in production (from my reading). That will likely make Sucrase's zero-configuration goal harder; if we use dev mode by default, it will completely break in production, and if we use prod mode by default, it will miss useful debugging info in dev.
  • The changes to the transform don't make it any more React-agnostic, which I was hoping to see at some point so that other libraries could be used without special configuration.
  • Simply having the old and new transforms will make Sucrase configuration a bit awkward. A possible approach is to make it opt-in in Sucrase 3, default in Sucrase 4, and drop the old transform in Sucrase 5, but there could be a more aggressive transition.

There are some positives with the new system, though:

  • Third-party library configuration will just be one string (importSource) rather than two.
  • We won't need to parse the configured pragma into a base and suffix (e.g. React and createElement) so they can be separately handled to account for import renaming.
@aleclarson
Copy link
Contributor

This would be awesome! I would definitely use it in vite-react-jsx, as using Babel makes build times ~40% longer for the ./demo folder. One thing to keep in mind: Modules using require should not have import {...} from "react/jsx-runtime" inserted, if possible. Otherwise, I'll have to manually rewrite them (using RegExp hacks) into require calls. Not a huge deal, but it would help if that was handled OOTB by Sucrase.

@alangpierce
Copy link
Owner Author

I'm finally getting around to this one. As mentioned, there are lots of open questions and details, so I wrote up a tech plan to help gather my thoughts https://github.com/alangpierce/sucrase/wiki/JSX-Automatic-Runtime-Transform-Technical-Plan

Here's a quick summary:

  • There will be a new jsxRuntime option that can be set to "automatic" to enable the new transform. It will be enabled by default in the future.
  • The existing production flag will be used to determine whether to use jsxDEV or jsx/jsxs. My current leaning is to make production: true the default in the future (when enabling the new JSX transform by default) to avoid surprise production crashes.
  • The jsxImportSource option can be used to customize the import, just like other tools allow.
  • For now, there will be a dev usability bug when a key prop has a value that spans multiple lines. The body of that JSX element will have output line numbers that are shifted compared with the input line numbers, so stack traces and debugger breakpoints will be off. The JSX-specific line numbers will still work. Multi-line key expressions are very rare, so this likely will be a minor issue.
  • One interesting thing I found as I was investigating this is that the automatic runtime transform was actually designed as an intermediate state for a long-term transition: Suggestion: transforming jsx to jsx-runtime without createElement fallback facebook/react#20031 (comment) . Hopefully the eventual future state will allow for a simpler implementation, even if this intermediate transform adds complexity.

alangpierce added a commit that referenced this issue Aug 16, 2022
Progress toward #585

This change extends the parser to detect two JSX cases that are needed for the
automatic runtime. The new code is run unconditionally, even in the old
transform, since flagging it would have its own overhead and complexity.

The new JSX transform needs to distinguish children being static or non-static,
where static is equivalent to having at least two comma-separated children
emitted by the old transform or any child being a spread child. The main
challenge in getting this right is that JSX whitespace-trimming will sometimes
completely remove all content from a text range, in which case it shouldn't
count as a child for the purposes of counting the number of children.
Fortunately, there is a relatively simple algorithm to detect if a text range
is empty: it's empty if and only if it is entirely whitespace and has at least
one newline. To identify these "empty text" regions, I added a new token type
`jsxEmptyText` that is treated the same except for the purposes of counting
children. In the future, it likely is reasonable to not treat such a region as a
token in the first place.

The new JSX transform also needs to detect the pattern of a key appearing after
a prop spread. We don't do keyword detection on JSX prop names, so instead I
manually detect the name "key", but only if we have already seen a prop spread.
alangpierce added a commit that referenced this issue Aug 16, 2022
Progress toward #585

This change extends the parser to detect two JSX cases that are needed for the
automatic runtime. The new code is run unconditionally, even in the old
transform, since flagging it would have its own overhead and complexity.

The new JSX transform needs to distinguish children being static or non-static,
where static is equivalent to having at least two comma-separated children
emitted by the old transform or any child being a spread child. The main
challenge in getting this right is that JSX whitespace-trimming will sometimes
completely remove all content from a text range, in which case it shouldn't
count as a child for the purposes of counting the number of children.
Fortunately, there is a relatively simple algorithm to detect if a text range
is empty: it's empty if and only if it is entirely whitespace and has at least
one newline. To identify these "empty text" regions, I added a new token type
`jsxEmptyText` that is treated the same except for the purposes of counting
children. In the future, it likely is reasonable to not treat such a region as a
token in the first place.

The new JSX transform also needs to detect the pattern of a key appearing after
a prop spread. We don't do keyword detection on JSX prop names, so instead I
manually detect the name "key", but only if we have already seen a prop spread.
alangpierce added a commit that referenced this issue Aug 19, 2022
Fixes #585

Tech plan:
https://github.com/alangpierce/sucrase/wiki/JSX-Automatic-Runtime-Transform-Technical-Plan

This PR adds two new options:
* `jsxRuntime`, which can be "classic" (the existing behavior) or "automatic"
  (the transform behavior released with React 17).
* `jsxImportSource`, which configures the import prefix when using the automatic
  runtime.

This PR also makes these options available for the website.
alangpierce added a commit that referenced this issue Sep 12, 2022
Follow-up to #585

This PR also resolves pre-existing key issues. I also tried a separate prod
build to confirm that `production: true` is necessary to avoid a runtime error.
alangpierce added a commit that referenced this issue Sep 13, 2022
Follow-up to #585

This PR also resolves pre-existing key issues. I also tried a separate prod
build to confirm that `production: true` is necessary to avoid a runtime error.
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 a pull request may close this issue.

2 participants