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

Fix: incorrect transform jsx-runtime in development #2741

Merged
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
8444297
fix(es_transforms_react): fix import behavior in dev mode
Shinyaigeek Nov 13, 2021
b5e80d9
chore: add test for jsx-dev-runtime
Shinyaigeek Nov 13, 2021
96087d1
Update: jsx_src not apply __source props in runtime automatic
Shinyaigeek Nov 14, 2021
54e34c6
Update: jsx_self not apply __self into props with runtime: automatic
Shinyaigeek Nov 14, 2021
77322cd
Update: chain source and self in jsxDev
Shinyaigeek Nov 14, 2021
03469b0
Update: apply source and self and isStaticValidation with jsxDev
Shinyaigeek Nov 14, 2021
a8b4564
Chore: update test for jsx-dev-runtime
Shinyaigeek Nov 14, 2021
7e0d292
Update: set __src and __self into props also in runtime: automatic
Shinyaigeek Nov 20, 2021
512287d
Chore: remove unncecessary break line
Shinyaigeek Nov 20, 2021
31c2085
Update: fix key handling
Shinyaigeek Nov 20, 2021
59275d6
Update: get __source and __self from props and set it into jsxDEV in …
Shinyaigeek Nov 20, 2021
798720c
Chore: add test for integration jsx, jsx_src, jsx_self
Shinyaigeek Nov 20, 2021
1969dca
Chore: mv jsx-dev-transform test
Shinyaigeek Nov 20, 2021
8f762f8
Refactor: replace String with str
Shinyaigeek Nov 21, 2021
a56ffa3
Refactor: avoied __ for __self and __source
Shinyaigeek Nov 21, 2021
6b13073
Refactor: use undefined util module
Shinyaigeek Nov 21, 2021
3526ff9
Refactor: replace Some -> once
Shinyaigeek Nov 21, 2021
0e831ae
Chore: add some test for jsx transforms
Shinyaigeek Nov 23, 2021
d09b3a9
fix(es_transforms_react): fix import behavior in dev mode
Shinyaigeek Nov 13, 2021
c2b2e5e
chore: add test for jsx-dev-runtime
Shinyaigeek Nov 13, 2021
4514808
Update: jsx_src not apply __source props in runtime automatic
Shinyaigeek Nov 14, 2021
841e746
Update: jsx_self not apply __self into props with runtime: automatic
Shinyaigeek Nov 14, 2021
04b436e
Update: chain source and self in jsxDev
Shinyaigeek Nov 14, 2021
faf413c
Update: apply source and self and isStaticValidation with jsxDev
Shinyaigeek Nov 14, 2021
2971268
Chore: update test for jsx-dev-runtime
Shinyaigeek Nov 14, 2021
0944196
Update: set __src and __self into props also in runtime: automatic
Shinyaigeek Nov 20, 2021
e5ebdd0
Chore: remove unncecessary break line
Shinyaigeek Nov 20, 2021
93902b5
Update: fix key handling
Shinyaigeek Nov 20, 2021
ed291e2
Update: get __source and __self from props and set it into jsxDEV in …
Shinyaigeek Nov 20, 2021
47a377d
Chore: add test for integration jsx, jsx_src, jsx_self
Shinyaigeek Nov 20, 2021
211d214
Chore: mv jsx-dev-transform test
Shinyaigeek Nov 20, 2021
3212e23
Refactor: replace String with str
Shinyaigeek Nov 21, 2021
cee7b47
Refactor: avoied __ for __self and __source
Shinyaigeek Nov 21, 2021
6aa3885
Refactor: use undefined util module
Shinyaigeek Nov 21, 2021
8829383
Refactor: replace Some -> once
Shinyaigeek Nov 21, 2021
e36fda1
Chore: add some test for jsx transforms
Shinyaigeek Nov 23, 2021
b1716df
Chore: add test case for Fragment jsxDev argument
Shinyaigeek Nov 30, 2021
2454c08
Merge branch 'fix/incorrect-import-jsx-development-runtime' of https:…
Shinyaigeek Nov 30, 2021
1004193
Chore: update other test case use Fragment with development mode
Shinyaigeek Nov 30, 2021
5cd46d9
Update: atatch key and isStaticChild into Fragment with development mode
Shinyaigeek Nov 30, 2021
5cae082
Chore: fmt
Shinyaigeek Nov 30, 2021
79f6e2e
Chore: add test case for fragment
Shinyaigeek Dec 1, 2021
8a8fc1e
Update: use undefined(DUMMY_SP) instread of Boxing
Shinyaigeek Dec 5, 2021
fdaaaa2
Chore: move jsxdev-args-with-gragment test from fixture into integration
Shinyaigeek Dec 5, 2021
5be5cf0
Update: use chain instead of vec and into_iter
Shinyaigeek Dec 8, 2021
5af8a79
Merge branch 'main' into fix/incorrect-import-jsx-development-runtime
kdy1 Dec 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 167 additions & 39 deletions crates/swc_ecma_transforms_react/src/jsx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use swc_common::{
use swc_ecma_ast::*;
use swc_ecma_parser::{Parser, StringInput, Syntax};
use swc_ecma_transforms_base::helper;
use swc_ecma_utils::{drop_span, member_expr, prepend, private_ident, quote_ident, ExprFactory};
use swc_ecma_utils::{
drop_span, member_expr, prepend, private_ident, quote_ident, undefined, ExprFactory,
};
use swc_ecma_visit::{as_folder, noop_visit_mut_type, Fold, VisitMut, VisitMutWith};

mod static_check;
Expand Down Expand Up @@ -205,6 +207,7 @@ where
pragma_frag: parse_expr_for_jsx(&cm, "pragmaFrag", options.pragma_frag, top_level_mark),
use_builtins: options.use_builtins,
use_spread: options.use_spread,
development: options.development,
throw_if_namespace: options.throw_if_namespace,
top_level_node: true,
})
Expand Down Expand Up @@ -237,6 +240,7 @@ where
pragma_frag: Arc<Box<Expr>>,
use_builtins: bool,
use_spread: bool,
development: bool,
throw_if_namespace: bool,
}

Expand Down Expand Up @@ -346,13 +350,14 @@ where

match self.runtime {
Runtime::Automatic => {
let jsx = if use_jsxs {
let jsx = if use_jsxs && !self.development {
self.import_jsxs
.get_or_insert_with(|| private_ident!("_jsxs"))
.clone()
} else {
let jsx = if self.development { "_jsxDEV" } else { "_jsx" };
self.import_jsx
.get_or_insert_with(|| private_ident!("_jsx"))
.get_or_insert_with(|| private_ident!(jsx))
.clone()
};

Expand Down Expand Up @@ -396,10 +401,26 @@ where
}
}

let args = once(fragment.as_arg()).chain(once(props_obj.as_arg()));

let args = if self.development {
args.chain(once(undefined(DUMMY_SP).as_arg()))
.chain(once(
Lit::Bool(Bool {
span: DUMMY_SP,
value: use_jsxs,
})
.as_arg(),
))
.collect()
} else {
args.collect()
};

Expr::Call(CallExpr {
span,
callee: jsx.as_callee(),
args: vec![fragment.as_arg(), props_obj.as_arg()],
args,
type_args: None,
})
}
Expand Down Expand Up @@ -452,13 +473,14 @@ where
self.import_create_element
.get_or_insert_with(|| private_ident!("_createElement"))
.clone()
} else if use_jsxs {
} else if use_jsxs && !self.development {
self.import_jsxs
.get_or_insert_with(|| private_ident!("_jsxs"))
.clone()
} else {
let jsx = if self.development { "_jsxDEV" } else { "_jsx" };
self.import_jsx
.get_or_insert_with(|| private_ident!("_jsx"))
.get_or_insert_with(|| private_ident!(jsx))
.clone()
};

Expand All @@ -468,6 +490,8 @@ where
};

let mut key = None;
let mut source_props = None;
let mut self_props = None;

for attr in el.opening.attrs {
match attr {
Expand All @@ -489,6 +513,44 @@ where
continue;
}

if !use_create_element
&& *i.sym == *"__source"
&& self.development
{
if source_props.is_some() {
panic!("Duplicate __source is found");
}
source_props = attr
.value
.map(jsx_attr_value_to_expr)
.flatten()
.map(|expr| ExprOrSpread { expr, spread: None });
assert_ne!(
source_props, None,
"value of property '__source' should not be empty"
);
continue;
}

if !use_create_element
&& *i.sym == *"__self"
&& self.development
{
if self_props.is_some() {
panic!("Duplicate __self is found");
}
self_props = attr
.value
.map(jsx_attr_value_to_expr)
.flatten()
.map(|expr| ExprOrSpread { expr, spread: None });
assert_ne!(
self_props, None,
"value of property '__self' should not be empty"
);
continue;
}

let value = match attr.value {
Some(v) => jsx_attr_value_to_expr(v)
.expect("empty expression container?"),
Expand Down Expand Up @@ -596,13 +658,52 @@ where

self.top_level_node = top_level_node;

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Nov 14, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Nov 20, 2021

Choose a reason for hiding this comment

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

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.

let args = once(name.as_arg()).chain(once(props_obj.as_arg()));
let args = if self.development {
// set undefined literal to key if key is None
let key = match key {
Some(key) => key,
None => ExprOrSpread {
spread: None,
expr: undefined(DUMMY_SP),
},
};

// set undefined literal to __source if __source is None
let source_props = match source_props {
Some(source_props) => source_props,
None => ExprOrSpread {
spread: None,
expr: undefined(DUMMY_SP),
},
};

// set undefined literal to __self if __self is None
let self_props = match self_props {
Some(self_props) => self_props,
None => ExprOrSpread {
spread: None,
expr: undefined(DUMMY_SP),
},
};
args.chain(once(key))
.chain(once(
Lit::Bool(Bool {
span: DUMMY_SP,
value: use_jsxs,
})
.as_arg(),
))
.chain(once(source_props))
.chain(once(self_props))
.collect()
} else {
args.chain(key).collect()
};
Expr::Call(CallExpr {
span,
callee: jsx.as_callee(),
args: once(name.as_arg())
.chain(once(props_obj.as_arg()))
.chain(key)
.collect(),
args,
type_args: Default::default(),
})
}
Expand Down Expand Up @@ -970,44 +1071,71 @@ where
);
}

let imports = self
.import_jsx
.take()
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("jsx")),
is_type_only: false,
})
.into_iter()
.chain(self.import_jsxs.take().map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("jsxs")),
is_type_only: false,
}))
.chain(
self.import_fragment
.take()
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("Fragment")),
is_type_only: false,
}),
)
.map(ImportSpecifier::Named)
.collect::<Vec<_>>();
let imports = self.import_jsx.take();
let imports = if self.development {
imports
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("jsxDEV")),
is_type_only: false,
})
.into_iter()
.chain(
self.import_fragment
.take()
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("Fragment")),
is_type_only: false,
}),
)
.map(ImportSpecifier::Named)
.collect::<Vec<_>>()
} else {
imports
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("jsx")),
is_type_only: false,
})
.into_iter()
.chain(self.import_jsxs.take().map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("jsxs")),
is_type_only: false,
}))
.chain(
self.import_fragment
.take()
.map(|local| ImportNamedSpecifier {
span: DUMMY_SP,
local,
imported: Some(quote_ident!("Fragment")),
is_type_only: false,
}),
)
.map(ImportSpecifier::Named)
.collect::<Vec<_>>()
};

if !imports.is_empty() {
let jsx_runtime = if self.development {
"jsx-dev-runtime"
} else {
"jsx-runtime"
};
prepend(
&mut module.body,
ModuleItem::ModuleDecl(ModuleDecl::Import(ImportDecl {
span: DUMMY_SP,
specifiers: imports,
src: Str {
span: DUMMY_SP,
value: format!("{}/jsx-runtime", self.import_source).into(),
value: format!("{}/{}", self.import_source, jsx_runtime).into(),
has_escape: false,
kind: Default::default(),
},
Expand Down
45 changes: 44 additions & 1 deletion crates/swc_ecma_transforms_react/src/jsx/tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![allow(dead_code)]

use super::*;
use crate::display_name;
use crate::{display_name, react};
use std::path::PathBuf;
use swc_common::{chain, Mark};
use swc_ecma_parser::EsConfig;
Expand Down Expand Up @@ -72,6 +72,28 @@ fn fixture_tr(t: &mut Tester, mut options: FixtureOptions) -> impl Fold {
display_name(),
)
}

fn integration_tr(t: &mut Tester, mut options: FixtureOptions) -> impl Fold {
let top_level_mark = Mark::fresh(Mark::root());

options.options.next = options.babel_8_breaking || options.options.runtime.is_some();

if !options.babel_8_breaking && options.options.runtime.is_none() {
options.options.runtime = Some(Runtime::Classic);
}

options.options.use_builtins |= options.use_builtins;
chain!(
resolver_with_mark(top_level_mark),
react(
t.cm.clone(),
Some(t.comments.clone()),
options.options,
top_level_mark
),
display_name(),
)
}
test!(
::swc_ecma_parser::Syntax::Es(::swc_ecma_parser::EsConfig {
jsx: true,
Expand Down Expand Up @@ -1385,3 +1407,24 @@ fn fixture(input: PathBuf) {
&output,
);
}

#[testing::fixture("tests/integration/fixture/**/input.js")]
fn integration(input: PathBuf) {
let mut output = input.with_file_name("output.js");
if !output.exists() {
output = input.with_file_name("output.mjs");
}

test_fixture_allowing_error(
Syntax::Es(EsConfig {
jsx: true,
..Default::default()
}),
&|t| {
let options = parse_options(input.parent().unwrap());
integration_tr(t, options)
},
&input,
&output,
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const App = (
<div>
<div />
<>
<div key={1}>hoge</div>
</>
</div>
);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{ "runtime": "automatic", "development": true }
Loading