Fix: incorrect transform `jsx-runtime` in development #2741

Shinyaigeek · 2021-11-13T19:49:33Z

Motivation 🔥

Currently, swc does not transform JSX correctly with new JSX Transform in development mode. It is importing jsx from jsx-runtime when it should be importing jsxDEV from jsx-dev-runtime. (The same applies to jsxs).

I fixed this.

kdy1

This is not enough.
You have to handle special attributes like __source, too.

Shinyaigeek · 2021-11-14T11:13:32Z

Oh! Sorry, I overlooked it 🙇

I updated code with passing isStaticValidation, source, self (this) to jsxDEV, and not set __source and __self as props with "runtime": "automatic" config.

kdy1

I think there's are small issues with jsx directives.

kdy1 · 2021-11-14T12:25:20Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

@@ -596,13 +600,62 @@ where

                self.top_level_node = top_level_node;



I think this code will error if /* @jsxRuntime automatic */ is used.

Because of this, the babel plugin detect attributes named __source and __self and change those into arguments.

I’m sorry for that I did not know about jsx directives.

Because of this, the babel plugin detect attributes named __source and __self and change those into arguments.

Is this the process below?

https://github.com/babel/babel/blob/main/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts#L502

I am worried if my understanding is correct, but should I fix the code to set __source and __self props as JSX attribute in development mode as usual?

Sorry for late response, I missed the email.

Is this the process below?

Yes.

Should I fix the code to set __source and __self props as JSX attribute in development mode as usual?

I think so, as it's what babel does

Thank you for your reply and help!

I fixed code like below

set __source and __self to props with jsx_src and jsx_self (as usual)

As babel plugin does, with runtime: automatic and development: true configuration, swc extract __source and __self from JSX’s attributes and set it as _jsxDEV’s arguments.

it looks like there are not test settings to test react module(= jsx with jsx_src and jsx_self) with a specific configuration, so I added integration testing directory to test react module with a specific configuration.

If there is yet something I missed, I will be happy if you pointed it out.

kdy1 · 2021-11-21T06:30:02Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

@@ -489,6 +495,44 @@ where
                                        continue;
                                    }

+                                    if !use_create_element
+                                        && i.sym == String::from("__source")


String::from("__source") is very inefficient.
You can compare JsWord to str like *i.sym === "__source ".

👍 1ed2c7b

crates/swc_ecma_transforms_react/src/jsx/mod.rs

kdy1 · 2021-11-21T06:30:49Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

@@ -468,6 +472,8 @@ where
                };

                let mut key = None;
+                let mut __source = None;


I think variable name should not use _ as a prefix as it has special meaning in rust.

👍 7fcafd2

I replaced __source with source_props, and __self with self_props.

crates/swc_ecma_transforms_react/src/jsx/mod.rs

kdy1 · 2021-11-21T06:32:14Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

+                        Some(key) => key,
+                        None => ExprOrSpread {
+                            spread: None,
+                            expr: Box::new(build_undefined_literal_value()),


Please use undefined(DUMMY_SP) instead.

I did not notice undefined util module 🙇

👍 0ac5ded

crates/swc_ecma_transforms_react/src/jsx/mod.rs

kdy1 · 2021-11-21T06:33:04Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

+                            })
+                            .as_arg(),
+                        ))
+                        .chain(Some(__source))


There's std::iter::once and it's better than using Some to create an iterator.

👍 75a12b8

crates/swc_ecma_transforms_react/src/jsx/mod.rs

kdy1

Thank you!

kdy1 · 2021-11-21T09:23:31Z

Code changes look fine to me, but can you add some more tests?
Actually this feature was once implemented by #2542, but I reverted it because it broke next.js

Shinyaigeek · 2021-11-23T10:30:29Z

Thank you!

I've added some tests on this, but I am wondering what more tests to add.

Can you share what happened or error logs when Next.js was broken? (I searched for the Next.js issue but couldn't find it)

kdy1 · 2021-11-23T10:34:34Z

@Shinyaigeek vercel/next.js#30302 (comment)
This was the issue, and thank you!

kdy1

Thanks!
I found some tiny issues.

crates/swc_ecma_transforms_react/src/jsx/mod.rs

kdy1 · 2021-12-01T09:29:23Z

crates/swc_ecma_transforms_react/tests/jsx/fixture/runtime/jsxdev-args-with-fragment/output.js

+    ]
+},
+void 0,
+true);


I got different output from babel.

var _jsxFileName = "/Users/kdy1/projects/lab/src/input.js"; import { jsxDEV as _jsxDEV } from "react/jsx-dev-runtime"; import { Fragment as _Fragment } from "react/jsx-dev-runtime"; var x = /*#__PURE__*/_jsxDEV(_Fragment, { children: [/*#__PURE__*/_jsxDEV("div", { children: "hoge" }, void 0, false, { fileName: _jsxFileName, lineNumber: 3, columnNumber: 9 }, this), /*#__PURE__*/_jsxDEV("div", { children: "fuga" }, void 0, false, { fileName: _jsxFileName, lineNumber: 4, columnNumber: 9 }, this)] }, void 0, true);

Seems like children of fragments are not visited?

Can you add e.visit_mut_children_with(self) to

swc/crates/swc_ecma_transforms_react/src/jsx_src/mod.rs

Line 38 in 79f6e2e

and

swc/crates/swc_ecma_transforms_react/src/jsx_self/mod.rs

Line 39 in 79f6e2e

I think this works. jsxdev-args-with-gragment test is placed in fixture, not in integration. This means jsx-src and jsx-self does not work while this test run. This output will be the same with babel’s output if we move this test into integration. I think this is a little confusing, so I moved this test case from fixture into integration in fdaaaa2

Shinyaigeek · 2021-12-05T18:27:57Z

I’m sorry I did not understand why #2542 broke next.js (vercel/next.js#30302 (comment)). I think there are enough test cases for this change, but I am also worried that this change will break next.js as vercel/next.js#30302 (comment) . Is there any documentation for running Next.js tests with the local version of swc? (I rewrote Cargo.toml of next-swc with {path = "..."}, I get a compile error ...)

kdy1 · 2021-12-06T06:20:15Z

@Shinyaigeek Same here. I don't understand the reason.

I think in this case it will be fine, as you added tests for all input. But if you want to try it, you can patch all swc dependencies in https://github.com/vercel/next.js/tree/c004322d138e1949437efb0b80e0973f3a3775cf/packages/next-swc/crates to use git or path dependency targeting your own branch and create a PR.

kdy1 · 2021-12-06T06:20:47Z

But I really think it will be fine in this case, as we tested all input.

Shinyaigeek · 2021-12-08T09:35:31Z

@kdy1 I see. It seems to be a tough task to test with next-swc, so is it okay to get a review as it is, and send a follow up issue & PR if there is any problem?

kdy1 · 2021-12-08T09:38:03Z

Yeah, I think it is better. Thanks for the pr again and I'll review it soon

kdy1

Thanks! I found one perf/code style issue.

kdy1 · 2021-12-08T14:22:08Z

crates/swc_ecma_transforms_react/src/jsx/mod.rs

+
+                let args = if self.development {
+                    args.into_iter()
+                        .chain(


I think .chain(once(undefined(DUMMY_SP).as_arg())).chain(once(Lit::Bool(Bool { span: DUMMY_SP, value: use_jsxs, }) .as_arg())) would be better.

OK 👍 Thank you for helping! I fixed in 5be5cf0

kdy1

LGTM. Thanks!

## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `yarn lint` This PR applies - swc-project/swc#2741 This fixes `development` mode of jsx.

)

## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `yarn lint` This PR applies - swc-project/swc#2741 This fixes `development` mode of jsx.

kdy1 added this to the v1.2.109 milestone Nov 13, 2021

kdy1 requested changes Nov 13, 2021

View reviewed changes

Shinyaigeek force-pushed the fix/incorrect-import-jsx-development-runtime branch from 19065d3 to e0fe330 Compare November 14, 2021 07:30

Shinyaigeek requested a review from kdy1 November 14, 2021 11:12

kdy1 requested changes Nov 14, 2021

View reviewed changes

kdy1 modified the milestones: v1.2.109, v1.2.110, v1.2.111, v1.2.112 Nov 17, 2021

Shinyaigeek force-pushed the fix/incorrect-import-jsx-development-runtime branch 2 times, most recently from fddc954 to 6351e01 Compare November 20, 2021 17:08

Shinyaigeek requested a review from kdy1 November 20, 2021 17:20

kdy1 reviewed Nov 21, 2021

View reviewed changes

crates/swc_ecma_transforms_react/src/jsx/mod.rs Outdated Show resolved Hide resolved

kdy1 reviewed Nov 21, 2021

View reviewed changes

crates/swc_ecma_transforms_react/src/jsx/mod.rs Outdated Show resolved Hide resolved

kdy1 reviewed Nov 21, 2021

View reviewed changes

crates/swc_ecma_transforms_react/src/jsx/mod.rs Outdated Show resolved Hide resolved

kdy1 reviewed Nov 21, 2021

View reviewed changes

crates/swc_ecma_transforms_react/src/jsx/mod.rs Outdated Show resolved Hide resolved

kdy1 reviewed Nov 21, 2021

View reviewed changes

crates/swc_ecma_transforms_react/src/jsx/mod.rs Outdated Show resolved Hide resolved

kdy1 requested changes Nov 21, 2021

View reviewed changes

Shinyaigeek requested a review from kdy1 November 21, 2021 08:25

kdy1 removed this from the v1.2.112 milestone Nov 24, 2021

kdy1 added this to the v1.2.116 milestone Dec 1, 2021

Chore: add test case for fragment

79f6e2e

kdy1 requested changes Dec 1, 2021

View reviewed changes

kdy1 modified the milestones: v1.2.116, v1.2.117, v1.2.118, v1.2.119 Dec 1, 2021

Shinyaigeek added 2 commits December 6, 2021 03:12

Update: use undefined(DUMMY_SP) instread of Boxing

8a8fc1e

Chore: move jsxdev-args-with-gragment test from fixture into integration

fdaaaa2

kdy1 requested changes Dec 8, 2021

View reviewed changes

Update: use chain instead of vec and into_iter

5be5cf0

kdy1 approved these changes Dec 10, 2021

View reviewed changes

Merge branch 'main' into fix/incorrect-import-jsx-development-runtime

5af8a79

kdy1 enabled auto-merge (squash) December 10, 2021 08:46

kdy1 merged commit ed704c9 into swc-project:main Dec 10, 2021

kdy1 mentioned this pull request Dec 11, 2021

Update jsx transform of swc vercel/next.js#32383

Merged

10 tasks

nicholasxjy pushed a commit to nicholasxjy/swc that referenced this pull request Dec 18, 2021

fix(es/react): Implement proper development handling (swc-project#2741

8b324cf

)

Shinyaigeek mentioned this pull request Dec 31, 2021

2021年のしにゃい振り返り Shinyaigeek/Shinyaigeek#2

Open

ShravanSunder mentioned this pull request Jun 20, 2022

Unable to set swc option for automatic react runtime TypeStrong/ts-node#1800

Closed

swc-project locked as resolved and limited conversation to collaborators Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: incorrect transform `jsx-runtime` in development #2741

Fix: incorrect transform `jsx-runtime` in development #2741

Shinyaigeek commented Nov 13, 2021

kdy1 left a comment

Shinyaigeek commented Nov 14, 2021 •

edited

Loading

kdy1 left a comment

kdy1 Nov 14, 2021

kdy1 Nov 14, 2021

Shinyaigeek Nov 14, 2021 •

edited

Loading

kdy1 Nov 20, 2021

Shinyaigeek Nov 20, 2021 •

edited

Loading

kdy1 Nov 21, 2021

Shinyaigeek Nov 21, 2021

kdy1 Nov 21, 2021

Shinyaigeek Nov 21, 2021 •

edited

Loading

kdy1 Nov 21, 2021

Shinyaigeek Nov 21, 2021

kdy1 Nov 21, 2021

Shinyaigeek Nov 21, 2021

kdy1 left a comment

kdy1 commented Nov 21, 2021

Shinyaigeek commented Nov 23, 2021

kdy1 commented Nov 23, 2021

kdy1 left a comment

kdy1 Dec 1, 2021

kdy1 Dec 1, 2021

Shinyaigeek Dec 5, 2021

Shinyaigeek commented Dec 5, 2021

kdy1 commented Dec 6, 2021

kdy1 commented Dec 6, 2021

Shinyaigeek commented Dec 8, 2021

kdy1 commented Dec 8, 2021

kdy1 left a comment

kdy1 Dec 8, 2021

Shinyaigeek Dec 8, 2021

kdy1 left a comment

		@@ -596,13 +600,62 @@ where

		self.top_level_node = top_level_node;

Fix: incorrect transform jsx-runtime in development #2741

Fix: incorrect transform jsx-runtime in development #2741

Conversation

Shinyaigeek commented Nov 13, 2021

Motivation 🔥

kdy1 left a comment

Choose a reason for hiding this comment

Shinyaigeek commented Nov 14, 2021 • edited Loading

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinyaigeek Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinyaigeek Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinyaigeek Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

kdy1 commented Nov 21, 2021

Shinyaigeek commented Nov 23, 2021

kdy1 commented Nov 23, 2021

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinyaigeek commented Dec 5, 2021

kdy1 commented Dec 6, 2021

kdy1 commented Dec 6, 2021

Shinyaigeek commented Dec 8, 2021

kdy1 commented Dec 8, 2021

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

Fix: incorrect transform `jsx-runtime` in development #2741

Fix: incorrect transform `jsx-runtime` in development #2741

Shinyaigeek commented Nov 14, 2021 •

edited

Loading

Shinyaigeek Nov 14, 2021 •

edited

Loading

Shinyaigeek Nov 20, 2021 •

edited

Loading

Shinyaigeek Nov 21, 2021 •

edited

Loading