Support JSX fragments with jsxFragmentFactory compiler option and @jsxFrag pragma

[nojvek]

Reviving: #35392 (which was closed due to inactivity)

Checklist

• There is an associated issue in the Backlog. Fixes Support JSX-Fragments with custom jsxFactory #20469
• Code is up-to-date with the master branch
• You've successfully run gulp runtests locally
• There are new or updated unit tests validating the change

Problem:

Currently <><Foo /></> only works with "jsx": "react". Using an inline pragma for jsxFactory /** @jsx dom */ or config defined "jsxFactory": "h" throws an error that JSX fragment is not supported when using --jsxFactory

The 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

1. jsxFragmentFactory compiler option.

As suggested and reviewed by @weswigham in previous PR.

Babel plugin-transform-react-jsx supports following options

{
  "plugins": [
    ["@babel/plugin-transform-react-jsx", {
      "pragma": "Preact.h", // default pragma is React.createElement
      "pragmaFrag": "Preact.Fragment", // default is React.Fragment
      "throwIfNamespace": false // defaults to true
    }]
  ]
}

Typescript already supports jsxFactory compiler option, this PR adds another optional jsxFragmentFactory compiler option for similar developer UX.

{
  "compilerOptions": {
    "target": "esnext", 
    "module": "commonjs",
    "jsx": "react",
    "jsxFactory": "h",
    "jsxFragmentFactory": "Fragment"
  }
}

2. support for @jsxFrag pragma. This code would work in both typescript and babel without changes. TS will need jsx: react for emit though.

/** @jsx Preact.h */
/** @jsxFrag Preact.Fragment */

import Preact from 'preact';

var descriptions = items.map(item => (
  <>
    <dt>{item.name}</dt>
    <dd>{item.value}</dd>
  </>
));

[nojvek]

@weswigham - would appreciate a review when you get a chance.

Thanks!

[nojvek]

@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 require

//// [mix-n-match.js]
"use strict";
exports.__esModule = true;
/* @jsx h */
/* @jsxFrag Frag */
var preact_1 = require("./preact");
preact_1.h(snabbdom_1.Frag, null,
    preact_1.h("span", null));

[weswigham]

You'll need to mark it as referenced when a fragment that uses it is check'ed in the checker - checkJsxOpeningLikeElementOrOpeningFragment does this. It needs to swap the name it resolves and marks when it's looking at a fragment.

[nojvek]

Sweet! I have the type elision scenario passing as a test.

@weswigham, ready for a review 👀 when you are.

[nojvek]

@typescript-bot pack this

[nojvek]

I guess @typescript-bot only listens to typescript team 🤷‍♂️

[weswigham]

@typescript-bot pack this :P

[typescript-bot]

Heya @weswigham, I've started to run the tarball bundle task on this PR at df5a194. You can monitor the build here.

[nojvek]

Please tell me that when a TS team member types @typescript-bot sudo make me a 🥪, something really cool happens. If not, you need to seriously evaluate priorities in the roadmap. Ha!

[typescript-bot]

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/74777/artifacts?artifactName=tgz&fileId=4E617F10A295CC8B5DD8371C8F44C745356B0A4C8893BB6AD7964773498EA80F02&fileName=/typescript-4.0.0-insiders.20200523.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[nojvek]

@weswigham do you have any ETA when you'll review this or whether it's likely to land in next TS version ?

[weswigham]

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.

[nojvek]

Kind ping @DanielRosenwasser for 👀

[nojvek]

Kind ping again @DanielRosenwasser for review 👀 ^

[nojvek]

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.

[DanielRosenwasser]

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.

[nojvek]

You are the best @DanielRosenwasser. Thank you 🙏

[weswigham]

@andrewbranch you wanna give this a second review and help get this merged?

[andrewbranch]

Approved, with grammar note. Thanks @nojvek!

[andrewbranch]

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” 🙃

[DanielRosenwasser]

Pretty glad we moved away from "working @microsoft" to "working at @microsoft"

[weswigham]

... I pronounce the @. 😆

[nojvek]

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 🤷

[andrewbranch]

Just curious, what’s the utility of this? I couldn’t find anything about using null in the babel transform docs.

[weswigham]

I think the idea is that some libraries may use a null 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...

[nojvek]

snabbdom-jsx-lite uses null. https://github.com/nojvek/snabbdom-jsx-lite/blob/master/tsconfig.json#L8

Mostly because if

is compiled to h('div', null). null meaning no attrs,

<> compiles to h(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.

[weswigham]

@nojvek would you be able to do a quick resync with master?

[nojvek]

FYI @weswigham, merged with latest master and ensured tests pass locally.