- Notifications
You must be signed in to change notification settings - Fork12.9k
Support JSX fragments with jsxFragmentFactory compiler option and @jsxFrag pragma#38720
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4010afc to975e7b8Compare@weswigham - would appreciate a review when you get a chance. Thanks! |
9927484 to30a8f17Compare@weswigham I added a type elision test like you mentioned. Did a bit of debugging but still have little idea how to make the emmiter emit jsxFrag import so we don't end up with this, where snabbdom doesn't have a corresponding //// [mix-n-match.js]"use strict";exports.__esModule=true;/*@jsx h *//*@jsxFrag Frag */varpreact_1=require("./preact");preact_1.h(snabbdom_1.Frag,null,preact_1.h("span",null)); |
You'll need to mark it as |
30a8f17 todf5a194CompareSweet! I have the type elision scenario passing as a test. @weswigham, ready for a review 👀 when you are. |
@typescript-bot pack this |
I guess@typescript-bot only listens to typescript team 🤷♂️ |
@typescript-bot pack this :P |
typescript-bot commentedMay 23, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the tarball bundle task on this PR atdf5a194. You can monitor the buildhere. |
Please tell me that when a TS team member types |
typescript-bot commentedMay 23, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey@weswigham, I've packed this intoan installable tgz. You can install it for testing by referencing it in your and then running There is also a playgroundfor this build. |
@weswigham do you have any ETA when you'll review this or whether it's likely to land in next TS version ? |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this looks pretty good now (small note above). I'd like to get another pair of eyes, just to be prudent, and confirm this is OK to merge with@DanielRosenwasser.
Kind ping@DanielRosenwasser for 👀 |
Kind ping again@DanielRosenwasser for review 👀 ^ |
Another kind ping@DanielRosenwasser ^ @weswigham is there anyone else in the TS team that can review this for merge? I have a feeling that this could be a PR that sits for months and eventually ends up being closed. This is my 3rd try trying to make a PR for jsxFragments over the last 2 years. Please please please don't leave this hanging in limbo. |
Hey, I checked in with a couple of other people on the team. I'm going to raise it for the design meeting agenda tomorrow, but if there's nothing else controversial I think we can get it in by next week for the release. |
You are the best@DanielRosenwasser. Thank you 🙏 |
@andrewbranch you wanna give this a second review and help get this merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approved, with grammar note. Thanks@nojvek!
| "category": "Error", | ||
| "code": 17016 | ||
| }, | ||
| "JSX fragmentisnot supportedwhen using aninline JSX factory pragma": { | ||
| "An @jsxFrag pragmaisrequiredwhen using an@jsx pragma with JSX fragments.": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hopefully this doesn’t turn into the GIF pronunciation debate, but I don’t pronounce the@ character so I think these articles should be “a,” not “an” 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pretty glad we moved away from "working @microsoft" to "working at @microsoft"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
... I pronounce the@. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When@weswigham first mentioned this as PR comment, I didn't fully agree with him, but when I read it over many times I was internally conflicted which way is the right way. In the end I chose to appease@weswigham 🤷
src/compiler/checker.ts Outdated
| const jsxFactoryNamespace = getJsxNamespace(node); | ||
| const jsxFactoryLocation = isNodeOpeningLikeElement ? (<JsxOpeningLikeElement>node).tagName : node; | ||
| // allow null as jsxFragmentFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just curious, what’s the utility of this? I couldn’t find anything about usingnull in the babel transform docs.
weswighamJun 15, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the idea is that some libraries may use anull tag name to indicate a fragment, rather than some specific fragment object.Mithril actually uses'[' as the fragment sentinel nowadays, so we should actually consider supporting arbitrary simple expressions here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
snabbdom-jsx-lite uses null.https://github.com/nojvek/snabbdom-jsx-lite/blob/master/tsconfig.json#L8
Mostly because if
h('div', null). null meaning no attrs,<> compiles toh(null, null) meaning it's no named tag i.e Fragment. This avoids an extra function call for evaluation of fragment function, which in most cases convert the children to an array.
@nojvek would you be able to do a quick resync with |
6a7a8c7 to2d7372aCompare2d7372a tob950086CompareFYI@weswigham, merged with latest master and ensured tests pass locally. |
Uh oh!
There was an error while loading.Please reload this page.
Reviving:#35392 (which was closed due to inactivity)
Checklist
Backlog.FixesSupport JSX-Fragments with custom jsxFactory #20469masterbranchgulp runtestslocallyProblem:
Currently
<><Foo /></>only works with"jsx": "react". Using an inline pragma for jsxFactory/** @jsx dom */or config defined"jsxFactory": "h"throws an error thatJSX fragment is not supported when using --jsxFactoryThe issue has been open for almost 3 years now.
Proposal Fix:
Very much inspired from babel to keep ecosystem consistent.
https://babeljs.io/docs/en/babel-plugin-transform-react-jsx
jsxFragmentFactorycompiler option.As suggested and reviewed by@weswigham in previous PR.
Babel plugin-transform-react-jsx supports following options
Typescript already supports
jsxFactorycompiler option, this PR adds another optionaljsxFragmentFactorycompiler option for similar developer UX.{"compilerOptions": {"target":"esnext","module":"commonjs","jsx":"react","jsxFactory":"h","jsxFragmentFactory":"Fragment" }}@jsxFragpragma. This code would work in both typescript and babel without changes. TS will need jsx:reactfor emit though.