fix(es/typescript): preserve Flow component type semantics - #12090
fix(es/typescript): preserve Flow component type semantics#12090labor0-bot[bot] wants to merge 13 commits into
Conversation
🦋 Changeset detectedLatest commit: 7f2ce67 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a36befc1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| parameters: self | ||
| .params | ||
| .into_iter() | ||
| .map(|p| p.babelify(ctx).into()) |
There was a problem hiding this comment.
For any Flow component(...) parsed by this commit, the parser stores the component props as a single TsFnParam::Object, but this ESTree conversion reuses the TsFnType path and calls Into<IdOrRest> for each param. IdOrRest only accepts identifiers and rest elements and panics on Object, so converting code such as type C = component(foo: string); through swc_estree_compat now aborts instead of producing an AST. Flatten or otherwise encode the component props before calling .into().
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| ObjectPatProp::Assign(prop) => emit!(prop), | ||
| ObjectPatProp::Rest(prop) => emit!(prop), |
There was a problem hiding this comment.
When a component type uses Flow's spread-prop rest syntax like component(...Props), the parser represents it as an ObjectPatProp::Rest with a synthetic component_rest binding and the real spread type in the rest type annotation. Emitting the RestPat here prints ...component_rest: Props, which is a named rest prop rather than the original spread-prop type, so codegen changes public Flow types for React Native declarations such as component(...AnimatedProps<Props>). Detect that synthetic spread-type form and emit ...<type> instead of the fallback binding.
Useful? React with 👍 / 👎.
| formatting_space!(emitter); | ||
| keyword!(emitter, "as"); | ||
| formatting_space!(emitter); | ||
| emit!(prop.value); |
There was a problem hiding this comment.
For Flow component props that are both optional and aliased, e.g. component(foo? as bar: string), the parser records the optional marker on the alias binding. Emitting the whole value here therefore produces component(foo as bar?: string), but this parser's component grammar only accepts ? before as, so codegen can generate Flow that immediately fails to reparse. When the key/value are not shorthand, the optional marker needs to be printed on the prop key side instead of inside the alias pattern.
Useful? React with 👍 / 👎.
| /// Returns whether a syntactic type annotation identifies a Flow component. | ||
| fn is_flow_component_type(ty: &TsType) -> bool { | ||
| match ty { | ||
| TsType::TsFnOrConstructorType(TsFnOrConstructorType::TsComponentType(_)) => true, |
There was a problem hiding this comment.
If a component-typed binding is nullable, such as const C: ?component() = () => null, Flow parsing represents it as a union containing TsComponentType, null, and undefined, so this predicate returns false and skips the arrow-to-function conversion. That leaves nullable component arrows without a usable .prototype, which is the same runtime case this change is trying to fix; handle the nullable union wrapper as a component annotation before falling through.
Useful? React with 👍 / 👎.
Merging this PR will degrade performance by 2.89%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | es/large/all/es2018 |
107.7 ms | 110.9 ms | -2.89% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/flow-component-type-semantics (7f2ce67) with main (394c7c9)2
Footnotes
-
61 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(29679ad) during the generation of this report, so 394c7c9 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29e5518952
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| **init = Expr::Fn(FnExpr { | ||
| ident: Some(id.clone()), |
There was a problem hiding this comment.
When a component-typed arrow is declared inside a class method and its body references lexical super, this rewrite moves the same body into a normal function expression, so Flow stripping emits invalid JavaScript such as function C(){ return super.x; }, whereas the original arrow was valid. Please either skip this lowering for arrows that contain super or rewrite in a way that preserves the lexical super binding.
Useful? React with 👍 / 👎.
| } else { | ||
| emit!(prop.key); | ||
| formatting_space!(emitter); | ||
| keyword!(emitter, "as"); | ||
| formatting_space!(emitter); | ||
| emit!(prop.value); |
There was a problem hiding this comment.
For a component type with a string-literal prop written without an alias, e.g. component("data-testid": string), the parser stores the key as PropName::Str and uses the synthetic component_prop fallback binding. This branch treats every non-identifier/shorthand key as an alias and emits "data-testid" as component_prop: string, so codegen changes public Flow declarations by exposing an implementation-only name. Detect the synthetic fallback for string keys and print the original "key": Type form instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96e4f18919
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| }; | ||
|
|
||
| **init = Expr::Fn(FnExpr { | ||
| ident: Some(id.clone()), |
There was a problem hiding this comment.
When a component-typed arrow is declared with let/var and its body reads the component binding after that binding can be reassigned, this turns let C: component() = () => C; const old = C; C = replacement; old() into a named function expression where C inside the body resolves to the function’s own inner name instead of the outer variable. The original arrow would observe the reassignment, while function C() { return C; } always returns itself; an anonymous function expression assigned to the declarator still gets a prototype/constructor, so avoid introducing the inner binding unless it is needed.
Useful? React with 👍 / 👎.
| let Expr::Arrow(arrow) = init.as_mut() else { | ||
| return; |
There was a problem hiding this comment.
For a component-typed binding whose initializer is a parenthesized arrow, e.g. const C: component() = (() => null);, SWC preserves the parentheses as Expr::Paren, so this guard returns before the new lowering runs. Flow stripping then still emits an arrow with no prototype, leaving the React Native mock prototype access broken solely because the initializer was parenthesized; unwrap transparent expression wrappers before checking for Expr::Arrow.
Useful? React with 👍 / 👎.
| formatting_space!(emitter); | ||
| keyword!(emitter, "as"); | ||
| formatting_space!(emitter); |
There was a problem hiding this comment.
When minifying an aliased component prop such as component(foo as bar: string), both formatting_space! calls are suppressed, so codegen writes component(fooasbar:string). That reparses as a single fooasbar prop instead of foo aliased to bar, silently changing exported Flow component types; use hard space! around the as keyword.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0889408dd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if !matches!( | ||
| self.type_ann.type_ann.as_ref(), | ||
| TsType::TsKeywordType(TsKeywordType { | ||
| kind: TsKeywordTypeKind::TsAnyKeyword, | ||
| .. |
There was a problem hiding this comment.
renders any annotations
When the input explicitly says renders any, parse_flow_component_renders_ann stores the same TsAnyKeyword that parse_flow_component_type uses as the synthetic default for a missing renders clause, so this check suppresses both cases. A declaration such as type C = component() renders any; is printed as type C = component();, losing the user-authored Flow render bound in generated public declarations; keep the presence of the renders clause separately or only elide the parser-inserted default.
Useful? React with 👍 / 👎.
| **init = Expr::Fn(FnExpr { | ||
| ident: Some(id.clone()), |
There was a problem hiding this comment.
this when lowering component arrows
For component-typed arrows that capture lexical this, for example inside a method with const C: component() = () => this.value, replacing the arrow with a normal function expression rebinds this from the call site instead of the enclosing method instance. That is a runtime regression for valid Flow-stripped JavaScript that previously preserved arrow lexical captures; skip or rewrite these arrows when they reference lexical this/arguments rather than converting them directly.
Useful? React with 👍 / 👎.
| swc_ecma_parser: major | ||
| swc_ecma_react_compiler: patch | ||
| swc_ecma_transforms_typescript: patch | ||
| swc_ecma_visit: patch |
There was a problem hiding this comment.
This changeset records swc_ecma_visit as a patch, but the same commit adds NodeRef::TsComponentType to the public NodeRef enum in crates/swc_ecma_visit/src/generated.rs, which downstream users can exhaustively match. Publishing this as a patch can ship a semver-breaking API under the existing major version; bump swc_ecma_visit as major or avoid changing that public enum.
AGENTS.md reference: AGENTS.md:L41-L41
Useful? React with 👍 / 👎.
| #[test] | ||
| fn issue_12045_component_arrow_supports_react_native_mock_access() { |
There was a problem hiding this comment.
This adds the React Native mock regression as an inline #[test], but the repository asks new coverage to prefer fixture suites; this case can live with the existing Flow strip fixtures instead of embedding source and assertions directly in the harness, and the same pattern also appears in the transform crate test added by this change.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
Donny/강동윤 (kdy1)
changed the title
Continue working with Labor0Submit a review from Files changed, or leave an inline review comment. Regular comments in Conversation do not trigger the agent. Reviews from collaborators with Write, Maintain, or Admin access are handled automatically. Ask a question for an explanation, or request a code change and Labor0 will evaluate it and update this pull request when appropriate. Successful change requests update the branch without a routine completion reply. Labor0 replies to questions and non-actionable feedback and resolves each processed inline review thread unless clarification is required. Automated review boilerplate paired with selected inline feedback does not receive a separate reply. Related feedback may be handled together, so wait for any necessary replies, new commits, and required checks before merging. No |
Donny/강동윤 (kdy1)
left a comment
Donny/강동윤 (kdy1)
left a comment
There was a problem hiding this comment.
Would it be possible to avoid breaking swc_ecma_ast?
labor0-bot
Bot
changed the title
|
Donny/강동윤 (@kdy1) Yes—addressed in 37e28bd. The implementation now keeps the existing TsFunctionType AST representation and removes the proposed TsComponentType variant and its downstream visitor, binding, and type changes. Component syntax is recognized privately by codegen and Flow stripping, with regression coverage ensuring ordinary destructured function types are not misclassified. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37e28bd4c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| **init = Expr::Fn(FnExpr { | ||
| ident: Some(id.clone()), | ||
| function: Box::new(Function { |
There was a problem hiding this comment.
When a component-typed arrow is declared inside a constructor or factory and its body reads new.target, this rewrite moves the body into a normal function expression, so new.target is rebound to the component call instead of the enclosing construct call. For example, function make(){ const C: component() = () => new.target; return C; } currently returns make from new make()(), but after Flow stripping it returns undefined unless the component itself is invoked with new; skip or rewrite arrows that capture new.target.
Useful? React with 👍 / 👎.
| if !is_flow_component_type(&type_ann.type_ann) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the component annotation is intersected with statics, e.g. const C: component() & {displayName?: string} = () => null, the parser represents the annotation as an intersection containing the component function type, so this check returns false and the arrow is left without a usable prototype. Flow component values commonly get intersected with static members, so unwrap/scan intersections for a component member before deciding not to apply the lowering.
Useful? React with 👍 / 👎.
| if self.flow_syntax { | ||
| convert_flow_component_arrow(n); | ||
| } |
There was a problem hiding this comment.
This only invokes the new lowering from variable declarators with a binding annotation, so a Flow component arrow typed by a cast, such as export default ((props) => null) as component(props: Props);, is later unwrapped by visit_mut_expr and emitted as an arrow with no prototype. Component casts are used for exported Flow components, so handle as component(...) / type-cast expression forms before the generic Flow-strip pass erases the annotation.
Useful? React with 👍 / 👎.
2 participants
Description:
Flow component annotations use the existing TsFunctionType AST representation, but the Flow stripping transform still needs to distinguish component-typed arrows from ordinary and hook-typed arrows so components receive normal function prototype semantics.
This change:
Validation completed:
The full parser suite retains one unrelated existing TypeScript fixture mismatch (import { type import }), and the full swc suite retains 13 Node 24 stacktrace snapshot whitespace mismatches.
Related issue (if exists):
Fixes #12045