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

feat: JSX Support #3038

Merged
merged 5 commits into from
Oct 2, 2019
Merged

feat: JSX Support #3038

merged 5 commits into from
Oct 2, 2019

Conversation

keroxp
Copy link
Contributor

@keroxp keroxp commented Sep 30, 2019

PR for #3037

Added JSX support feature. Now .jsx .tsx files can be imported.
By default, there is no jsxFactory function at runtime. Default jsxFactory value of tsconfig.json is React.createElement. To use jsx/tsx in Deno, define or import react before JSX expression or define your own React.createElement or use custom jsxFactory with tsconfig.json:

import React from "https://dev.jspm.io/react"
import ReactDOMServer from "https://dev.jspm.io/react-dom/server"
const str = ReactDOMServer.renderToString(<div className="deno">land</div>);
console.log(str);
// => <div class="deno" data-reactroot="">land</div>

@ry
Copy link
Member

ry commented Sep 30, 2019

@keroxp JSX support is desirable - but is it necessary to add these top level methods to Deno? renderToString, Component, etc... Those seem like framework level needs.

Also, please add an integration test to cli/tests/integration_tests.rs

@keroxp
Copy link
Contributor Author

keroxp commented Sep 30, 2019

@ry Not necessary but may be convenient for developers. Ok I'll remove default jsx factories from this PR anyway.

@keroxp keroxp changed the title feat: JSX Support [WIP] feat: JSX Support Sep 30, 2019
@keroxp keroxp changed the title [WIP] feat: JSX Support feat: JSX Support Sep 30, 2019
@@ -0,0 +1,7 @@
function h(factory, props, ...children) {
return {factory, props, children}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not linted properly. We should ensure the linting covers these files as well.

tsconfig.json Outdated
@@ -17,6 +17,8 @@
"sourceMap": true,
"strict": true,
"target": "esnext",
"jsx": "react",
"jsxFactory": "h",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned we are not using the default here of React.createElement. I can understand why, but it will be potentially surprising to users that we aren't using the default. At the very least I would like to see updates to the manual along with this PR before we merge it explaining how it is anticipated to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think either is OK. React.creaetElement may rather be useful for real use. react and react-dom/server can be imported in Deno via dev.jspm.io.

cli/state.rs Outdated
state_.ts_compiler.compile_async(state_.clone(), &out)
}
msg::MediaType::JavaScript => {
msg::MediaType::JavaScript | msg::MediaType::JSX => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this. This will leave the .jsx untouched if the checkJs compiler flag is not set. I can't see that being desirable.

We should have tests around .jsx and we should have an expected behaviour of how they are handled. We also need it consistent with how the compiler behaves, where by default we have allow JS but not check JS, so how does that effect the emit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I absolutely forgot it. Hmm... should we compile .jsx files as TSX?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, not compile as TSX, more that we will always attempt to transpile them, move it to the previous match clause, but I would check that that works as expected, when the checkJs compiler flag is not enabled (which is the default), which the integration test should prove.

@keroxp
Copy link
Contributor Author

keroxp commented Oct 1, 2019

@ry Added integration tests for jsx / tsx file. Both will be compiled same as ts and changed default jsxFactory to React.createElement . So can you check it?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - i look forward to doing react in deno

@ry ry merged commit d32f39f into denoland:master Oct 2, 2019
@ry ry mentioned this pull request Oct 2, 2019
@PopovMP
Copy link

PopovMP commented Oct 7, 2019

JSX was the last thing I expected in Deno core. Highly disappointed (((
So we discard NPM but include React. Probably the next will be $ or Vue or Laravel...
I was looking at Deno as a new pure tool without any bad legacy.

Wish you luck!
unfollow

@ry
Copy link
Member

ry commented Oct 7, 2019

@PopovMP Deno doesn't include React - JSX support is part of the TypeScript compiler - this is just exposing that feature. If you notice this PR only added 61 lines (which is mostly the integration tests).

@GrosSacASac
Copy link

Does it increase compile time for non-JSX files as well ?

@ry
Copy link
Member

ry commented Oct 7, 2019

@GrosSacASac No - we did not observe any change in our compile time benchmarks.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 8, 2019

@GrosSacASac it basically allowed something through that we didn't allow before. The capability has always been there in the TypeScript compiler, basically all that needed to be done was to interpret the media types properly.

While this feature doesn't make sense in and of itself for server side workloads, it does in the context of server side rendering. Of course end users could load a transpiler for JSX/TSX in the runtime and deal with all the module resolution, or we could just enable a feature already there. It makes sense to do the latter. I personally strongly dislike JSX, but it would be stilly to specifically hobble the compiler support to enforce that opinion on others.

We plan to support a public compiler API (see: #1739) and that would have allowed people to support it directly in Deno anyways, though some sort of custom compiler. Again it would be silly to force users to do that when it is already natively supported.

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 this pull request may close these issues.

5 participants