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 for other JSX factories #6146

Merged
merged 3 commits into from
Jan 7, 2016
Merged

Support for other JSX factories #6146

merged 3 commits into from
Jan 7, 2016

Conversation

rwyborn
Copy link

@rwyborn rwyborn commented Dec 18, 2015

PR for Issue #3788

Current implementation adds a new jsxNamespace compile option that can specify the JSX namespace (factory) when jsx mode is react.

--jsx react --jsxNamespace MyDOMLib

results in emits being MyDOMLib.createElement

I was thinking an alternative would be to just use the existing --jsx option where "preserve" and "react" are special reserved values. In this setup the above would be achieved by just doing

-- jsx MyDOMLib

Rowan Wyborn added 2 commits December 18, 2015 21:56
- added jsxNamespace compile option
- when jsx mode is "react", jsxNamespace optionally specifies the emit namespace for React calls, eg "--jsxNamespace MyDOMLib" will emit calls as MyDOMLib.createElement (instead of React.createElement)
- symbol specified by jsxNamespace must be present, else compile error is generated (same handling as is done for React symbol when no jsxNamespace is specified)
@msftclas
Copy link

Hi @rwyborn, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@RyanCavanaugh
Copy link
Member

I think we need to tweak the naming a bit here. This change is still very useful but doesn't directly address #3788 because it still requires that the library have the same shape (.createElement) as React.

I'm thinking:

let x = <Foo />;
  • --jsx preserve
    • let x = <Foo />;
  • --jsx react
    • let x = React.createElement(Foo);
  • --jsx react --reactNamespace rt
    • let x = rt.createElement(Foo);
  • --jsx make
    • let x = make(Foo);

@DanielRosenwasser
Copy link
Member

If you have --jsx make, why exactly do you need --reactNamespace rt?

@unional
Copy link
Contributor

unional commented Dec 18, 2015

I would suggest to "pollute the global" with a meaningful namespace, such as JSX.make() or JSX.compose(). But simple make() might work too, just that it is a bit less convenience when I customize it in code (i.e. couldn't have a proper singleton such as JSX to hold states, when needed).

@rwyborn
Copy link
Author

rwyborn commented Dec 18, 2015

@RyanCavanaugh I had exactly the same thought originally re: your --jsx make example. The problem is this doesn't define what to do with the JSX spread handling, which emits a call to React.__spread. So my compromise was to let the namespace be configured instead, ie --jsxNamespace MyDOMHandler emits

  • MyDOMHandler.createElement
  • MyDOMHandler.__spread

Client code can then map those to whatever custom library implementation it wants.

@tinganho
Copy link
Contributor

Namespace for me, means only MyDOMHandler and not with the method name MyDOMHandler.createElement. createElement is occupying the namespace of MyDOMHandler so itself is not a namespace IMO.

What about?

--react for preserving current behavior of --jsx react.
--jsxElementFactory [value] for defining the factory function for JSX.

I think the option --jsx was a little bit ambiguous. And since --jsx preserve doesn't make so much sense anymore when this got landed maybe deprecate --jsx option?

@unional unional mentioned this pull request Jan 2, 2016
@RyanCavanaugh
Copy link
Member

Sorry for the delay.

Discussed with @billti and @mhegazy and we landed on the following

  • This PR is 👍 (see one comment below)
  • More complex changes to emit semantics will wait until @rbuckton tree-transform-based emitter is checked in. At that point we can re-evaluate (e.g. we may end up with fully-pluggable JSX transforms)

The only change I'd like to see here (other than removal of merge conflicts 😉) is that jsxNamespace should be reactNamepsace since this is specific to React emit (and I don't want confusion with the real namespace JSX { which is not changing names).

Thanks!

@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

@rwyborn please change the option name to reactNamespace and refresh the PR, and we should be good to go.

@mhegazy mhegazy merged commit 2395890 into microsoft:master Jan 7, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

merged manually after updating the name and adding tests.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

thanks @rwyborn!

@rwyborn
Copy link
Author

rwyborn commented Jan 8, 2016

@mhegazy I just got back from holiday, thanks for fixing this up!

@unional
Copy link
Contributor

unional commented Nov 21, 2016

More complex changes to emit semantics will wait until @rbuckton tree-transform-based emitter is checked in. At that point we can re-evaluate (e.g. we may end up with fully-pluggable JSX transforms)

How is that doing?
It is this one? #6987

@mhegazy
Copy link
Contributor

mhegazy commented Nov 21, 2016

@unional looks like you are looking for #12135

@unional
Copy link
Contributor

unional commented Nov 21, 2016

Thank! I'll take a look at it.

It's been a while ago, I need to refresh what is my original needs. 🌷

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants