Fix defaultProps with JSX optimization

6 files changed, 73 insertions, 34 deletions

diff --git a/src/js_parser.zig b/src/js_parser.zig
index 4ef43865e..894775877 100644
--- a/src/js_parser.zig
+++ b/src/js_parser.zig
@@ -1028,6 +1028,8 @@ const StaticSymbolName = struct {
         pub const effect = NewStaticSymbol("effect");
         pub const delegateEvents = NewStaticSymbol("delegateEvents");
         pub const Solid = NewStaticSymbol("Solid");
+
+        pub const __merge = NewStaticSymbol("__merge");
     };
 };
 
@@ -5102,6 +5104,9 @@ fn NewParser_(
             if (p.options.features.jsx_optimization_inline) {
                 p.resolveGeneratedSymbol(&p.react_element_type);
                 p.resolveGeneratedSymbol(&p.es6_symbol_global);
+                if (p.runtime_imports.__merge) |*merge| {
+                    p.resolveGeneratedSymbol(merge);
+                }
             }
             p.resolveGeneratedSymbol(&p.jsx_runtime);
             p.resolveGeneratedSymbol(&p.jsxs_runtime);
@@ -13709,32 +13714,20 @@ fn NewParser_(
                                             if (props.items.len == 0) {
                                                 props_expression = p.e(E.Binary{ .op = Op.Code.bin_logical_or, .left = defaultProps, .right = props_object }, defaultProps.loc);
                                             } else {
-                                                // we assume that most components don't use defaultProps
-                                                // props: !MyComponent.defaultProps ? {myProp: 123} : { ...MyComponent.defaultProps,  myProp: 123}
-                                                var with_default_props = p.allocator.alloc(G.Property, props_object.data.e_object.properties.len + 1) catch unreachable;
-                                                with_default_props[0] = G.Property{
-                                                    .key = null,
-                                                    .value = defaultProps,
-                                                    .kind = Property.Kind.spread,
-                                                    .flags = Flags.Property.init(
-                                                        .{
-                                                            .is_spread = true,
-                                                        },
-                                                    ),
+                                                var call_args = p.allocator.alloc(Expr, 2) catch unreachable;
+                                                call_args[0..2].* = .{
+                                                    props_object,
+                                                    defaultProps,
                                                 };
-
-                                                std.mem.copy(G.Property, with_default_props[1..], props_object.data.e_object.properties.slice());
-                                                props_expression = p.e(
-                                                    E.If{
-                                                        .test_ = p.e(E.Unary{ .op = .un_not, .value = defaultProps }, defaultProps.loc),
-                                                        .no = p.e(
-                                                            E.Object{ .properties = G.Property.List.init(with_default_props), .close_brace_loc = e_.close_tag_loc },
-                                                            tag.loc,
-                                                        ),
-                                                        .yes = props_object,
-                                                    },
-                                                    props_object.loc,
-                                                );
+                                                // __merge(props, MyComponent.defaultProps)
+                                                // originally, we always inlined here
+                                                // see https://twitter.com/jarredsumner/status/1534084541236686848
+                                                // but, that breaks for defaultProps
+                                                // we assume that most components do not have defaultProps
+                                                // so __merge quickly checks if it needs to merge any props
+                                                // and if not, it passes along the props object
+                                                // this skips an extra allocation
+                                                props_expression = p.callRuntime(tag.loc, "__merge", call_args);
                                             }
                                         }
 
@@ -13785,11 +13778,6 @@ fn NewParser_(
                                             expr.loc,
                                         );
 
-                                        if (p.options.features.jsx_optimization_hoist) {
-                                            // if the expression is free of side effects
-                                            if (p.exprCanBeRemovedIfUnused(&props_object)) {}
-                                        }
-
                                         return output;
                                     } else {
                                         // -- The typical jsx automatic transform happens here --
@@ -15418,6 +15406,27 @@ fn NewParser_(
             return false;
         }
 
+        /// This is based on exprCanBeRemovedIfUnused. 
+        /// The main difference: identifiers, functions, arrow functions cause it to return false
+                    //
+                    // They could technically have side effects if the imported module is a
+                    // CommonJS module and the import item was translated to a property access
+                    // (which esbuild's bundler does) and the property has a getter with side
+                    // effects.
+                    //
+                    // But this is very unlikely and respecting this edge case would mean
+                    // disabling tree shaking of all code that references an export from a
+                    // CommonJS module. It would also likely violate the expectations of some
+                    // developers because the code *looks* like it should be able to be tree
+                    // shaken.
+                    //
+                    // So we deliberately ignore this edge case and always treat import item
+                    // references as being side-effect free.
+                    return true;
+                },
+                .e_if => |ex| {
+                    return p.exprCanBeHoistedForJSX(&ex.test_) and
+                        (p.isSideEffectFreeUnboundIdentifierRef(
         fn isSideEffectFreeUnboundIdentifierRef(p: *P, value: Expr, guard_condition: Expr, is_yes_branch: bool) bool {
             if (value.data != .e_identifier or
                 p.symbols.items[value.data.e_identifier.ref.innerIndex()].kind != .unbound or
diff --git a/src/js_printer.zig b/src/js_printer.zig
index 8d7e404f3..df1517ae5 100644
--- a/src/js_printer.zig
+++ b/src/js_printer.zig
@@ -134,10 +134,6 @@ pub fn quoteForJSON(text: []const u8, output_: MutableString, comptime ascii_onl
             }
         }
         switch (c) {
-            // Special-case the bell character since it may cause dumping this file to
-            // the terminal to make a sound, which is undesirable. Note that we can't
-            // use an octal literal to print this shorter since octal literals are not
-            // allowed in strict mode (or in template strings).
             0x07 => {
                 try bytes.appendSlice("\\x07");
                 i += 1;
diff --git a/src/runtime.footer.bun.js b/src/runtime.footer.bun.js
index 9415ede08..c04d83ed3 100644
--- a/src/runtime.footer.bun.js
+++ b/src/runtime.footer.bun.js
@@ -11,6 +11,7 @@ export var __cJS2eSM = BUN_RUNTIME.__cJS2eSM;
 export var regeneratorRuntime = BUN_RUNTIME.regeneratorRuntime;
 export var __exportValue = BUN_RUNTIME.__exportValue;
 export var __exportDefault = BUN_RUNTIME.__exportDefault;
+export var __merge = BUN_RUNTIME.__merge;
 export var $$bun_runtime_json_parse = JSON.parse;
 export var __internalIsCommonJSNamespace =
   BUN_RUNTIME.__internalIsCommonJSNamespace;
diff --git a/src/runtime.footer.js b/src/runtime.footer.js
index 7a309647c..be4516473 100644
--- a/src/runtime.footer.js
+++ b/src/runtime.footer.js
@@ -18,6 +18,7 @@ export var __cJS2eSM = BUN_RUNTIME.__cJS2eSM;
 export var regeneratorRuntime = BUN_RUNTIME.regeneratorRuntime;
 export var __exportValue = BUN_RUNTIME.__exportValue;
 export var __exportDefault = BUN_RUNTIME.__exportDefault;
+export var __merge = BUN_RUNTIME.__merge;
 export var $$bun_runtime_json_parse = JSON.parse;
 export var __internalIsCommonJSNamespace =
   BUN_RUNTIME.__internalIsCommonJSNamespace;
diff --git a/src/runtime.js b/src/runtime.js
index 485cdc6f6..efb0265e4 100644
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -1,5 +1,6 @@
 var $$mod$ = Symbol.for;
 var __create = Object.create;
+var __descs = Object.getOwnPropertyDescriptors;
 var __defProp = Object.defineProperty;
 var __getProtoOf = Object.getPrototypeOf;
 var __hasOwnProp = Object.prototype.hasOwnProperty;
@@ -182,3 +183,26 @@ export var __reExport = (target, module, desc) => {
         });
   return target;
 };
+
+function hasAnyProps(obj) {
+  for (let key in obj) return true;
+  return false;
+}
+
+function mergeDefaultProps(props, defaultProps) {
+  var result = __create(defaultProps, __descs(props));
+
+  for (let key in defaultProps) {
+    if (result[key] !== undefined) continue;
+
+    result[key] = defaultProps[key];
+  }
+  return result;
+}
+export var __merge = (props, defaultProps) => {
+  return !hasAnyProps(defaultProps)
+    ? props
+    : !hasAnyProps(props)
+    ? defaultProps
+    : mergeDefaultProps(props, defaultProps);
+};
diff --git a/src/runtime.zig b/src/runtime.zig
index b59d1a2af..33561f9ce 100644
--- a/src/runtime.zig
+++ b/src/runtime.zig
@@ -339,6 +339,7 @@ pub const Runtime = struct {
         __exportValue: ?GeneratedSymbol = null,
         __exportDefault: ?GeneratedSymbol = null,
         __FastRefreshRuntime: ?GeneratedSymbol = null,
+        __merge: ?GeneratedSymbol = null,
 
         pub const all = [_][]const u8{
             // __HMRClient goes first
@@ -358,6 +359,7 @@ pub const Runtime = struct {
             "__exportValue",
             "__exportDefault",
             "__FastRefreshRuntime",
+            "__merge",
         };
         pub const Name = "bun:wrap";
         pub const alt_name = "bun:wrap";
@@ -452,6 +454,11 @@ pub const Runtime = struct {
                                 return Entry{ .key = 14, .value = val.ref };
                             }
                         },
+                        15 => {
+                            if (@field(this.runtime_imports, all[15])) |val| {
+                                return Entry{ .key = 15, .value = val.ref };
+                            }
+                        },
                         else => {
                             return null;
                         },
@@ -511,6 +518,7 @@ pub const Runtime = struct {
                 12 => (@field(imports, all[12]) orelse return null).ref,
                 13 => (@field(imports, all[13]) orelse return null).ref,
                 14 => (@field(imports, all[14]) orelse return null).ref,
+                15 => (@field(imports, all[15]) orelse return null).ref,
                 else => null,
             };
         }