JSX preserve mode: fix "make is not a valid component name" #7831

cknitt · 2025-09-02T09:57:26Z

Closes #7432 by disabling two optimization when in JSX preserve mode.

~~Ideally, the optimization should only be disabled for actual JSX expressions, but I have not found how to do that yet.~~

pkg-pr-new · 2025-09-02T10:20:45Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7831

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7831

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7831

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7831

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7831

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7831

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7831

commit: bd76cf1

nojaf

Thanks for your continued efforts to resolve this.

Could you add a new test case using the example from #7432?

I also think this code no longer makes sense because of that change:

rescript/compiler/syntax/src/jsx_v4.ml

Lines 96 to 108 in a09a3d7

    
           (* Build a string representation of a module name with segments separated by $ *) 
        
           let make_module_name file_name nested_modules fn_name = 
        
             let full_module_name = 
        
               match (file_name, nested_modules, fn_name) with 
        
               (* TODO: is this even reachable? It seems like the fileName always exists *) 
        
               | "", nested_modules, "make" -> nested_modules 
        
               | "", nested_modules, fn_name -> List.rev (fn_name :: nested_modules) 
        
               | file_name, nested_modules, "make" -> file_name :: List.rev nested_modules 
        
               | file_name, nested_modules, fn_name -> 
        
                 file_name :: List.rev (fn_name :: nested_modules) 
        
             in 
        
             let full_module_name = String.concat "$" full_module_name in 
        
             full_module_name

Transforming make to FileName$ModuleName appears irrelevant now since that won't be called anymore.

Related test: https://github.com/cknitt/rescript/blob/a1e7cc545c0da894aa3f992ae1b783fe684af220/tests/tests/src/jsx_preserve_test.mjs#L147C28-L160

Given this, should this behavior become the default regardless of preserve JSX? If so, the work in #7203 could be undone.

compiler/core/lam_pass_remove_alias.ml

cknitt · 2025-09-02T13:14:02Z

Thanks for your continued efforts to resolve this.

Could you add a new test case using the example from #7432?

I also think this code no longer makes sense because of that change:

rescript/compiler/syntax/src/jsx_v4.ml

Lines 96 to 108 in a09a3d7

(* Build a string representation of a module name with segments separated by $ *)

let make_module_name file_name nested_modules fn_name =

let full_module_name =

match (file_name, nested_modules, fn_name) with

(* TODO: is this even reachable? It seems like the fileName always exists *)

| "", nested_modules, "make" -> nested_modules

| "", nested_modules, fn_name -> List.rev (fn_name :: nested_modules)

| file_name, nested_modules, "make" -> file_name :: List.rev nested_modules

| file_name, nested_modules, fn_name ->

file_name :: List.rev (fn_name :: nested_modules)

in

let full_module_name = String.concat "$" full_module_name in

full_module_name

Transforming make to FileName$ModuleName appears irrelevant now since that won't be called anymore.

Related test: https://github.com/cknitt/rescript/blob/a1e7cc545c0da894aa3f992ae1b783fe684af220/tests/tests/src/jsx_preserve_test.mjs#L147C28-L160

Given this, should this behavior become the default regardless of preserve JSX? If so, the work in #7203 could be undone.

The uppercase function name is relevant for the React devtools. Otherwise all your components will have the name "make" there.

Also, React Compiler does not recognize lowercase component functions.

nojaf · 2025-09-02T13:54:43Z

I assumed wrongly that the React compiler would follow <C.make> and figure out it is a component.

function make(props) {
  return null;
}

let C = {
  make: make
};

let c = <C.make />;

The C in the jsx expression being uppercase, doesn't change anything.
function make needs to be uppercase before it will trigger.

nojaf

I'll leave this to @cristianoc. I can't quite tell whether it's harmless, so I suggested a few changes to tighten it up. Think of them as ideal-world tips since I'm not sure they're all feasible.

nojaf · 2025-09-02T15:56:32Z

compiler/core/js_pass_flatten_and_mark_dead.ml

@@ -245,6 +245,10 @@ let subst_map (substitution : J.expression Hash_ident.t) =
               turn a runtime crash into compile time crash : )
            *)
            match Ext_list.nth_opt ls (Int32.to_int i) with
+            (* 7432: prevent optimization in JSX preserve mode *)
+            | Some {expression_desc = J.Var (Id {name = "make"})}


This check could in theory affect each make function right?
Could we add some sort of check if that make returns a Jsx.element type? Just to further limit the scope when this kicks in.

there's no type this down the compiler stack

nojaf · 2025-09-02T15:57:47Z

compiler/core/lam_pass_remove_alias.ml

+    (* 7432: prevent optimization in JSX preserve mode *)
+    | Lprim
+        {
+          primitive = Pjs_call {prim_name = "jsx" | "jsxs"} as primitive;


Same remark, very unlikely that jsx is not going to come from ReactRuntime call, if we can further tighten it that be great.

I think it's plenty guarded against risk already

why not the same check that preserve mode is on?
I guess it's kind of implicit, and only a super corner case would have the same name, but since it's so easy to add why not

The check for preserve mode is already there (below).

nojaf · 2025-09-02T16:00:46Z

tests/tests/src/jsx_preserve_test.mjs

@@ -224,6 +223,47 @@ let _youtube_iframe = <iframe
  referrerPolicy={"strict-origin-when-cross-origin"}
 />;

+function make(_props) {


Thanks for adding the tests!

No longer valid.

cristianoc

looks plenty safe -- good to go for me
nice fix!

cristianoc · 2025-09-02T16:08:19Z

compiler/core/lam_pass_remove_alias.ml

+    (* 7432: prevent optimization in JSX preserve mode *)
+    | Lprim
+        {
+          primitive = Pjs_call {prim_name = "jsx" | "jsxs"} as primitive;


I think it's plenty guarded against risk already

cristianoc · 2025-09-02T16:09:47Z

compiler/core/lam_pass_remove_alias.ml

+    (* 7432: prevent optimization in JSX preserve mode *)
+    | Lprim
+        {
+          primitive = Pjs_call {prim_name = "jsx" | "jsxs"} as primitive;


why not the same check that preserve mode is on?
I guess it's kind of implicit, and only a super corner case would have the same name, but since it's so easy to add why not

cknitt · 2025-09-02T16:24:07Z

I tested against a large project, and the changes looked fine to me.

BTW, if I remove the checks for the JSX preserve mode and do make test, I only have a single change in the test output.

JSX preserve mode: fix "make is not a valid component name"

a1e7cc5

nojaf previously requested changes Sep 2, 2025

View reviewed changes

compiler/core/lam_pass_remove_alias.ml Outdated Show resolved Hide resolved

cknitt added 3 commits September 2, 2025 17:14

More selective fix for lam_pass_remove_alias

fc38b25

Selective fix for js_pass_flatten_and_mark_dead

9f12bcf

Add tests

941b636

cknitt force-pushed the jsx-preserve-fix branch from 0d4cc14 to 941b636 Compare September 2, 2025 15:20

cknitt requested review from cristianoc and nojaf September 2, 2025 15:39

nojaf reviewed Sep 2, 2025

View reviewed changes

cristianoc approved these changes Sep 2, 2025

View reviewed changes

CHANGELOG

bd76cf1

cknitt merged commit 2dc0103 into rescript-lang:master Sep 2, 2025
25 checks passed

cknitt deleted the jsx-preserve-fix branch September 2, 2025 17:04

	(* Build a string representation of a module name with segments separated by $ *)
	let make_module_name file_name nested_modules fn_name =
	let full_module_name =
	match (file_name, nested_modules, fn_name) with
	(* TODO: is this even reachable? It seems like the fileName always exists *)
	\| "", nested_modules, "make" -> nested_modules
	\| "", nested_modules, fn_name -> List.rev (fn_name :: nested_modules)
	\| file_name, nested_modules, "make" -> file_name :: List.rev nested_modules
	\| file_name, nested_modules, fn_name ->
	file_name :: List.rev (fn_name :: nested_modules)
	in
	let full_module_name = String.concat "$" full_module_name in
	full_module_name

JSX preserve mode: fix "make is not a valid component name" #7831

JSX preserve mode: fix "make is not a valid component name" #7831

Uh oh!

Conversation

cknitt commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cknitt commented Sep 2, 2025

Uh oh!

nojaf commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt commented Sep 2, 2025

Uh oh!

Uh oh!

Uh oh!

cknitt commented Sep 2, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 2, 2025 •

edited

Loading

nojaf commented Sep 2, 2025 •

edited

Loading