refactor(es/react): Inline dev jsx metadata into jsx hook#11533
refactor(es/react): Inline dev jsx metadata into jsx hook#11533
Conversation
|
kdy1
changed the title
kdy1
changed the title
kdy1
changed the title
Binary Sizes
Commit: 917da36 |
|
I expected more perf gain, but it seems like this is not very helpful |
|
Reopening as it has improvement of 1% |
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
PR Review: refactor(es/react): Inline dev jsx metadata into jsx hookSummaryThis PR inlines the Code Quality & CorrectnessThe inlining is faithful to the originals. I compared the new code against
Stale comment at line 1420-1423: /// By doing this in exit_expr (after children are visited), we ensure that
/// jsx_src and jsx_self have already added their __source and __self
/// attributes.
This comment references the old pipeline where Behavioral AnalysisHook ordering change in Old order: New order: This is correct because:
Automatic runtime ( Classic runtime (
PerformanceThis is a net positive for performance:
Minor note: Potential Issues
Test CoverageThe PR description indicates the following test suites were run:
This is adequate coverage. The existing fixture tests for VerdictClean refactoring that correctly consolidates three hooks into one. The only actionable item is the stale comment at line 1420-1423. The rest is solid. |
There was a problem hiding this comment.
Pull request overview
Refactors the React transform pipeline to inline development JSX metadata generation (__source, __self) into the main JSX hook, removing the need for separate jsx_src / jsx_self passes while keeping React Refresh and other hooks in the preset.
Changes:
- Reordered the
react()composite hook pipeline to remove standalonejsx_src/jsx_selfhooks and rely onjsx::hookfor dev metadata. - Added dev-mode
__source/__selfgeneration directly insidejsx/mod.rs, including constructor/derived-class suppression logic for__self. - Updated classic-runtime JSX element lowering to append dev attrs just before folding attributes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/swc_ecma_transforms_react/src/lib.rs | Removes jsx_src / jsx_self from the react() preset hook composition and simplifies the composite hook chain. |
| crates/swc_ecma_transforms_react/src/jsx/mod.rs | Implements dev __source / __self injection inside the JSX transform and adds context tracking to mirror prior jsx_self behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn append_dev_attrs(&self, attrs: &mut Vec<JSXAttrOrSpread>, opening_span: Span) { | ||
| if !self.development { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Dev-only __source/__self injection is now implemented inside the JSX transform (source_object_expr/append_dev_attrs/should_inject_self) instead of via separate passes. There don’t appear to be JSX transform tests asserting the generated __source/__self output (or the constructor/derived-class suppression behavior) for the integrated react() pipeline. Adding a focused test case would help prevent regressions in this newly inlined logic (including the createElement fallback path).
| if let Some(source) = self.source_object_expr(opening_span) { | ||
| props_obj | ||
| .props | ||
| .push(PropOrSpread::Prop(Box::new(Prop::KeyValue(KeyValueProp { | ||
| key: PropName::Ident(quote_ident!("__source")), | ||
| value: source, | ||
| })))); | ||
| } | ||
|
|
||
| if self.should_inject_self() { |
There was a problem hiding this comment.
In the use_create_element development branch, __source/__self are appended unconditionally to props_obj. If the original JSX already included __source/__self attributes, the earlier loop will already have emitted those keys into props_obj, and this later injection will create duplicate keys (and override the user-provided value because it is appended last). Consider checking whether props_obj already contains these keys (or tracking booleans while folding attrs) before pushing the dev metadata entries.
| if let Some(source) = self.source_object_expr(opening_span) { | |
| props_obj | |
| .props | |
| .push(PropOrSpread::Prop(Box::new(Prop::KeyValue(KeyValueProp { | |
| key: PropName::Ident(quote_ident!("__source")), | |
| value: source, | |
| })))); | |
| } | |
| if self.should_inject_self() { | |
| let has_source_prop = props_obj.props.iter().any(|prop_or_spread| { | |
| if let PropOrSpread::Prop(prop) = prop_or_spread { | |
| if let Prop::KeyValue(kv) = &**prop { | |
| if let PropName::Ident(ident) = &kv.key { | |
| return ident.sym == *"__source"; | |
| } | |
| } | |
| } | |
| false | |
| }); | |
| let has_self_prop = props_obj.props.iter().any(|prop_or_spread| { | |
| if let PropOrSpread::Prop(prop) = prop_or_spread { | |
| if let Prop::KeyValue(kv) = &**prop { | |
| if let PropName::Ident(ident) = &kv.key { | |
| return ident.sym == *"__self"; | |
| } | |
| } | |
| } | |
| false | |
| }); | |
| if let Some(source) = self.source_object_expr(opening_span) { | |
| if !has_source_prop { | |
| props_obj | |
| .props | |
| .push(PropOrSpread::Prop(Box::new(Prop::KeyValue(KeyValueProp { | |
| key: PropName::Ident(quote_ident!("__source")), | |
| value: source, | |
| })))); | |
| } | |
| } | |
| if self.should_inject_self() && !has_self_prop { |
| fn exit_static_block(&mut self, _n: &mut StaticBlock, _ctx: &mut ()) { | ||
| self.pop_ctx(); | ||
| } | ||
|
|
||
| /// Called after visiting children of an expression. |
There was a problem hiding this comment.
The doc comment for exit_expr just below this point still says jsx_src/jsx_self have already added __source/__self. After inlining dev metadata generation into the JSX hook, that comment is now misleading and should be updated/removed to match the new flow.
|
I'll review this later |
Summary
Testing