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 JSX fragments with jsxFragFactory compiler option and @jsxFrag pragma #35392

Closed
wants to merge 2 commits into from
Closed

Support JSX fragments with jsxFragFactory compiler option and @jsxFrag pragma #35392

wants to merge 2 commits into from

Conversation

nojvek
Copy link
Contributor

@nojvek nojvek commented Nov 27, 2019

Fixes #20469

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 2 years now.

Proposal Fix:

Very much inspired from babel to keep ecosystem consistent.

https://babeljs.io/docs/en/babel-plugin-transform-react-jsx

  1. jsxFragFactory compiler option.

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 jsxFragFactory compiler option for similar developer UX.

{
  "compilerOptions": {
    "target": "esnext", 
    "module": "commonjs",
    "jsx": "react",
    "jsxFactory": "Preact.h",
    "jsxFragFactory": "Preact.Fragment"
  }
}
  1. 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
Copy link
Contributor Author

nojvek commented Nov 27, 2019

fyi @RyanCavanaugh / marvinhagemeister / mmichlin66

Thoughts?

@uniqueiniquity
Copy link
Contributor

With a given jsxFactory jf, are you suggesting that TypeScript emit jf(null, null, ...children) and provide some way for the chosen JSX-supporting library to understand that at runtime? As far as I know, null is not commonly supported in that way - for example, Preact expects the exported symbol Fragment as shown here - so without the latter piece, this would fail at runtime in Preact. I'm not sure that it makes sense for TypeScript to emit something that the intended library doesn't support, and it seems like that's what you've proposed here.

@uniqueiniquity
Copy link
Contributor

See here for how Preact would handle this emitted code.

@nojvek
Copy link
Contributor Author

nojvek commented Dec 9, 2019

@uniqueiniquity that makes sense.

How about this, babel supports both jsx pragma and jsxFrag pragma. We could do it in a similar fashion. https://babeljs.io/docs/en/babel-plugin-transform-react-jsx

So for custom jsx factory if there is no pragma defined, then typescript checker will fail with an error, and if there is one defined, it will emit to that, just like jsxFactory.

jsxFrag pragma could be set to "null" and a library to could handle null if they wished.

This would mean Typescript and Babel behave in a similar fashion while supporting custom jsx factories with fragments.

@RyanCavanaugh ^ this works with you ?

@nojvek nojvek changed the title Support JSX-Fragments with custom jsxFactory as ${jsxFactory}(null, null, ...children) Support JSX fragments with jsxFragFactory compiler option and @jsxFrag pragma Jan 6, 2020
@nojvek
Copy link
Contributor Author

nojvek commented Jan 6, 2020

Take 2.

@uniqueiniquity / @RyanCavanaugh thoughts on the jsxFrag compiler option and pragma approach ^

If approach looks good to you, I'll add in more baseline tests.

@uniqueiniquity
Copy link
Contributor

@nojvek This approach seems much better to me.

cc: @weswigham - any thoughts?

@weswigham
Copy link
Member

We added support for @jsx pragmas to match babel, so it makes sense for the @jsxFrag pragma to exist and to also match.

But, suggestion: for the compiler option, jsxFragFactory -> jsxFragmentFactory

A Frag is a name for a kill in a video game, and the length of the compiler option doesn't really matter, since it's only set once in a config file, so it's better to not abbreviate unnecessarily.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In checkJsxOpeningLikeElementOrOpeningFragment we need to switch the symbol we look up from the "jsx namespace" as determined by the factory to the "jsx namespace" as determined by the fragment factory if we're checking a fragment and the fragment factory has been provided. Otherwise import ellision may erroneously occur.
Here's a test:

/* @jsx createElement */
/* @jsxFrag Fragment */
import { createElement, Fragment } from "react";
const fragment = <><h1>text</h1></>;

if the Fragment import is elided, the result would error at runtime. A strange case like

/* @jsx createElement */
/* @jsxFrag Fragment */
import { createElement } from "react";
import { Fragment } from "preact";
const fragment = <><h1>text</h1></>;

should also be tested. In that case, I imagine we'll throw checker errors on providing a react element to a preact fragment, but we should still preserve both imports in the .js emit.

@sandersn sandersn added this to Gallery in Pall Mall Jan 30, 2020
@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@sandersn sandersn added this to Waiting on author in PR Backlog Mar 5, 2020
@sandersn sandersn removed this from Gallery in Pall Mall Mar 5, 2020
@masx200
Copy link

masx200 commented Mar 8, 2020

This issue has not been resolved for a long time since the question was asked.
Is there any other plugin to solve this problem?

@nojvek
Copy link
Contributor Author

nojvek commented Mar 8, 2020

Sorry folks for dropping the ball. I quit my job earlier this year to work full time on a little startup. Since I have 0 income coming in, this has been pretty low on my priority list.

Feel free to pick it up. It may take me a month or two to get back to this. I didn’t realize how involved this would be.

@weswigham weswigham added the Help Wanted You can do this label Mar 10, 2020
@sandersn
Copy link
Member

I'm trying to keep the number of open PRs manageable, so I think I'll close this PR since it sounds like it's going to be dormant for at least a couple of months.

Of course we can re-open any time, or somebody new can use the commits here as a basis for a new PR. I'll link to this PR in the issue telling people to start new attempts here.

@sandersn sandersn closed this Mar 11, 2020
PR Backlog automation moved this from Waiting on author to Done Mar 11, 2020
@sandersn sandersn added For Backlog Bug PRs that fix a backlog bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 11, 2020
@sarimarton
Copy link

I'm trying to keep the number of open PRs manageable, so I think I'll close this PR since it sounds like it's going to be dormant for at least a couple of months.

Interesting criterion for closing an issue

@ForsakenHarmony
Copy link

Yeah, it's a bit weird, a close to me is either merged or rejected, but I guess typescript is quite big so stuff gets handled differently

@nojvek
Copy link
Contributor Author

nojvek commented Mar 19, 2020

I am doing a lot of work with preact and it's paining me so I will resume the PR shortly. Hopefully it won't take long to get it approved & merged.

@dead-claudia
Copy link

@sarimarton It's a PR, not an issue. Bit different.

@ForsakenHarmony In general on GitHub, it's not uncommon to close dormant PRs, as they're prone to get out of date on projects with any significant activity, and the more the PR gets out of sync due to other commits against the main repo, the more likely it's going to run into merge conflicts. So it just makes sense to close it out and wait for them to either update it so you can reopen it or just file a new PR with an up-to-date patch (if it takes long enough). I don't normally close pull requests this recent, but I can understand the reasoning in doing it - it is 4 months old after all and TS is a very active project for its size.

@rajuashok
Copy link

Has anyone found a workaround for this?

@nojvek
Copy link
Contributor Author

nojvek commented Sep 21, 2020

@rajuashok --jsxFragmentFactory compiler option has landed in TS4. No need for workaround. Upgrade to typescript 4

@rajuashok
Copy link

Ah I see! Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug Help Wanted You can do this
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Support JSX-Fragments with custom jsxFactory
9 participants