bundler: keep the CommonJS wrapper when code holds a lifted module's default import as a value

docs/bundler/index.mdx

@@ -1415,7 +1415,7 @@ In each case the member access compiles to a direct reference to `object`, the `
 
 Assignments (`z.x = 1`), optional chains, and non-literal computed keys (`z[key]`) are left as property accesses; `z["object"]` is treated like `z.object`. `export default someImport` is followed only when it ends at a namespace; a default that snapshots a `let` export keeps snapshot semantics.
 
-The same applies to the default import of a CommonJS module whose `exports.x = ...` assignments the bundler lifted to ES module exports, such as `react` and `scheduler`. The default import of a CommonJS module is its `module.exports`, which is that module's namespace, so `import React from "react"; React.useState()` compiles to a direct call of the lifted `useState` binding. The namespace object is created only when `React` itself is used as a value, and it lists the exports in assignment order, like `module.exports` does. A write through it, `React.useLayoutEffect = React.useEffect`, assigns the lifted binding, so every importer sees the new value, the same as a write to `module.exports`. `React.default`, and `ns.default` on `import * as ns`, is the lifted `default` export when the module has one, and otherwise the namespace itself, as `module.exports` is in Node. A module that sets both `exports.__esModule` and `exports.default` keeps its CommonJS wrapper when the importer is not an ES module by type (`.mjs`, `.mts`, or `"type": "module"`), because the default import then depends on that flag at run time. An `import()` of a module that does not set both resolves to that same namespace object as `default`, with or without code splitting.
+The same applies to the default import of a CommonJS module whose `exports.x = ...` assignments the bundler lifted to ES module exports, such as `react` and `scheduler`. The default import of a CommonJS module is its `module.exports`, which is that module's namespace, so `import React from "react"; React.useState()` compiles to a direct call of the lifted `useState` binding. This holds while every use of `React` reads, calls or assigns one of its properties: `React.useState`, `React[key]`, `const { useState } = React`. A write, `React.useLayoutEffect = React.useEffect`, goes through a namespace object whose setters assign the lifted binding, so every importer sees the new value, the same as a write to `module.exports`. Code that holds `React` itself as a value (`fn(React)`, `Object.keys(React)`, `const R = React`, a re-export) or deletes one of its properties can change the object in ways a namespace object cannot pass on to the lifted bindings (`Object.defineProperty`, `delete`, `Object.freeze`). The module then keeps its CommonJS wrapper and the default import is the real `module.exports` object. This rule looks at `import React from` only. `ns.default` on `import * as ns`, the `default` of an `import()` and the `this` of a call like `React.fn()` are still the namespace object of a module that stays lifted. `React.default`, and `ns.default` on `import * as ns`, is the lifted `default` export when the module has one, and otherwise the namespace itself, as `module.exports` is in Node. A module that sets both `exports.__esModule` and `exports.default` keeps its CommonJS wrapper when the importer is not an ES module by type (`.mjs`, `.mts`, or `"type": "module"`), because the default import then depends on that flag at run time. An `import()` of a module that does not set both resolves to that same namespace object as `default`, with or without code splitting.
 
 ### deprecatedNamespaceObjectSetters
 

src/ast/symbol.rs

@@ -116,6 +116,9 @@ bitflags::bitflags! {
         /// Renaming can also break any identifier used inside a "with" statement.
         const MUST_NOT_BE_RENAMED = 1 << 2;
 
+        /// An import some use holds as a value or deletes from, not only `X.a`, `X[a]`, `const { a } = X`.
+        const IMPORT_USED_AS_VALUE = 1 << 3;
+
         const REMOVE_OVERWRITTEN_FUNCTION_DECLARATION = 1 << 4;
 
         /// The file assigns this variable after its declaration (or a mapped
@@ -151,6 +154,7 @@ macro_rules! symbol_flag_accessors {
 symbol_flag_accessors! {
     must_start_with_capital_letter_for_jsx, set_must_start_with_capital_letter_for_jsx => MUST_START_WITH_CAPITAL_LETTER_FOR_JSX;
     must_not_be_renamed, set_must_not_be_renamed => MUST_NOT_BE_RENAMED;
+    import_used_as_value, set_import_used_as_value => IMPORT_USED_AS_VALUE;
     remove_overwritten_function_declaration, set_remove_overwritten_function_declaration => REMOVE_OVERWRITTEN_FUNCTION_DECLARATION;
     has_been_assigned_to, set_has_been_assigned_to => HAS_BEEN_ASSIGNED_TO;
     called_as_method, set_called_as_method => CALLED_AS_METHOD;

src/bundler/linker_context/scanImportsAndExports.rs

@@ -172,7 +172,11 @@ pub(crate) fn scan_imports_and_exports(
             // Named static imports contribute exactly their aliases to the
             // importee's observable-export set (see below); `* as ns` and
             // `export * from` make everything observable.
-            for ni in col_ref!(named_imports)[id].values() {
+            for (import_ref, ni) in col_ref!(named_imports)[id]
+                .keys()
+                .iter()
+                .zip(col_ref!(named_imports)[id].values())
+            {
                 let Some(record) = col_ref!(import_records_list)[id]
                     .as_slice()
                     .get(ni.import_record_index as usize)
@@ -183,9 +187,27 @@ pub(crate) fn scan_imports_and_exports(
                     continue;
                 }
                 let other = record.source_index.get() as usize;
-                if other >= col_ref!(exports_kind).len()
-                    || col_ref!(exports_kind)[other] != ExportsKind::Esm
+                if other >= col_ref!(exports_kind).len() {
+                    continue;
+                }
+                // `Object.freeze`, `defineProperty` and `delete` on a held default import need the real `module.exports`.
+                if col_ref!(ast_flags_list)[other].contains(AstFlags::COMMONJS_LIFTED_TO_ESM)
+                    && !ni.alias_is_star
+                    && ni.alias.is_some_and(|alias| alias.slice() == b"default")
+                    && this
+                        .graph
+                        .symbols
+                        .get_const(*import_ref)
+                        .is_some_and(|symbol| {
+                            // `ns.default` can be the module's own `default` export, which only step 3 resolves.
+                            symbol.import_item_status != bun_ast::ImportItemStatus::Generated
+                                && (ni.is_exported || symbol.import_used_as_value())
+                        })
                 {
+                    col!(exports_kind)[other] = ExportsKind::Cjs;
+                    col!(flags)[other].wrap = WrapKind::Cjs;
+                }
+                if col_ref!(exports_kind)[other] != ExportsKind::Esm {
                     continue;
                 }
                 // The default import of a lifted CommonJS module is its namespace.

src/js_parser/fold.rs

@@ -285,6 +285,9 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                                         .with_assign_target(identifier_opts.assign_target())
                                         .with_is_call_target(identifier_opts.is_call_target())
                                         .with_is_delete_target(identifier_opts.is_delete_target())
+                                        .with_is_property_access_target(
+                                            identifier_opts.is_property_access_target(),
+                                        )
                                         // If this expression is used as the target of a call expression, make
                                         // sure the value of "this" is preserved.
                                         .with_was_originally_identifier(false),

src/js_parser/p.rs

@@ -2138,6 +2138,13 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         }
     }
 
+    /// See `Symbol::import_used_as_value`.
+    fn note_import_use(&mut self, ref_: Ref, opts: IdentifierOpts) {
+        if !opts.is_property_access_target() && !self.is_control_flow_dead {
+            self.symbols[ref_.inner_index() as usize].set_import_used_as_value(true);
+        }
+    }
+
     pub(crate) fn log_arrow_arg_errors(&mut self, errors: &mut DeferredArrowArgErrors) {
         if errors.invalid_expr_await.len > 0 {
             let r = errors.invalid_expr_await;
@@ -2263,12 +2270,14 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                     }
                 }
 
+                self.note_import_use(ref_, opts);
                 return self.new_expr(E::ImportIdentifier::new(ident.ref_, true), loc);
             }
         }
 
         // Substitute an EImportIdentifier now if this is an import item
         if self.is_import_item.contains_key(&ref_) {
+            self.note_import_use(ref_, opts);
             return self.new_expr(
                 E::ImportIdentifier::new(ref_, opts.was_originally_identifier()),
                 loc,
@@ -5492,7 +5501,9 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 .with_must_keep_due_to_with_stmt(result.is_inside_with_scope)
                 .with_can_be_removed_if_unused(true),
             Some(parts[0]),
-            IdentifierOpts::new().with_was_originally_identifier(true),
+            IdentifierOpts::new()
+                .with_was_originally_identifier(true)
+                .with_is_property_access_target(parts.len() > 1),
         );
         if parts.len() > 1 {
             return Ok(self.member_expression(loc, value, &parts[1..]));
@@ -5529,14 +5540,13 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
     ) -> Expr {
         let mut value = initial_value;
 
-        for part in parts {
+        for (i, part) in parts.iter().enumerate() {
             if let Some(rewrote) = self.maybe_rewrite_property_access(
                 loc,
                 value,
                 part,
                 loc,
-                // All defaults on the packed-u8 IdentifierOpts.
-                IdentifierOpts::default(),
+                IdentifierOpts::default().with_is_property_access_target(i + 1 < parts.len()),
             ) {
                 value = rewrote;
             } else {
@@ -7916,6 +7926,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             M::MIdentifier(ref_) => {
                 self.record_usage(ref_);
                 let e = if self.is_import_item.contains_key(&ref_) {
+                    self.note_import_use(ref_, IdentifierOpts::new());
                     self.new_expr(
                         E::ImportIdentifier {
                             ref_,

src/js_parser/parser.rs

@@ -923,6 +923,7 @@ impl IdentifierOpts {
     const WAS_ORIGINALLY_IDENTIFIER: u8 = 1 << 3;
     const IS_CALL_TARGET: u8 = 1 << 4;
     const IS_TEMPLATE_TAG: u8 = 1 << 5;
+    const IS_PROPERTY_ACCESS_TARGET: u8 = 1 << 6;
 
     #[inline]
     pub(crate) const fn assign_target(self) -> js_ast::AssignTarget {
@@ -955,6 +956,11 @@ impl IdentifierOpts {
     pub(crate) const fn is_template_tag(self) -> bool {
         self.0 & Self::IS_TEMPLATE_TAG != 0
     }
+    /// See `ExprIn::is_property_access_target`.
+    #[inline]
+    pub(crate) const fn is_property_access_target(self) -> bool {
+        self.0 & Self::IS_PROPERTY_ACCESS_TARGET != 0
+    }
 
     // Builder-style helpers (this stays a packed u8 rather than a
     // named-field struct).
@@ -987,6 +993,11 @@ impl IdentifierOpts {
         self.0 = (self.0 & !Self::IS_TEMPLATE_TAG) | ((v as u8) << 5);
         self
     }
+    #[inline]
+    pub(crate) const fn with_is_property_access_target(mut self, v: bool) -> Self {
+        self.0 = (self.0 & !Self::IS_PROPERTY_ACCESS_TARGET) | ((v as u8) << 6);
+        self
+    }
 }
 
 pub(crate) fn statement_cares_about_scope(stmt: &Stmt) -> bool {
@@ -1041,6 +1052,9 @@ pub struct ExprIn {
     pub(crate) is_immediately_assigned_to_decl: bool,
 
     pub(crate) property_access_for_method_call_maybe_should_replace_with_undefined: bool,
+
+    /// The parent only reads, calls or assigns a property of this: `x.a`, `x[a]`, `const { a } = x`, not `delete x.a`.
+    pub(crate) is_property_access_target: bool,
 }
 
 /// This function exists to tie all of these checks together in one place

src/js_parser/visit/mod.rs

@@ -356,6 +356,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                     &mut val,
                     ExprIn {
                         is_immediately_assigned_to_decl: true,
+                        // `const { a } = x` reads `x.a` and drops `x`.
+                        is_property_access_target: matches!(decl.binding.data, BData::BObject(_)),
                         ..Default::default()
                     },
                 );

src/js_parser/visit/visit_expr.rs

@@ -334,7 +334,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                                 .ref_,
                         ),
                 )
-                .with_was_originally_identifier(true),
+                .with_was_originally_identifier(true)
+                .with_is_property_access_target(in_.is_property_access_target),
         );
     }
     // PERF(port:frame): keep these large, infrequently-taken arms out of the
@@ -883,7 +884,13 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             }
         }
 
-        p.visit_expr_in_out(&mut e_.target, ExprIn::default());
+        p.visit_expr_in_out(
+            &mut e_.target,
+            ExprIn {
+                is_property_access_target: !is_delete_target,
+                ..Default::default()
+            },
+        );
 
         match e_.index.data {
             Data::EPrivateIdentifier(mut private) => {
@@ -1008,7 +1015,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                                 .with_is_call_target(is_call_target)
                                 .with_is_template_tag(is_template_tag)
                                 .with_is_delete_target(is_delete_target)
-                                .with_assign_target(in_.assign_target);
+                                .with_assign_target(in_.assign_target)
+                                .with_is_property_access_target(in_.is_property_access_target);
                             if let Some(rewrite) = p.maybe_rewrite_property_access(
                                 expr.loc,
                                 e_.target,
@@ -1393,6 +1401,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             ExprIn {
                 property_access_for_method_call_maybe_should_replace_with_undefined: in_
                     .property_access_for_method_call_maybe_should_replace_with_undefined,
+                is_property_access_target: !is_delete_target,
                 ..Default::default()
             },
         );
@@ -1433,7 +1442,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 .with_is_call_target(is_call_target)
                 .with_is_template_tag(is_template_tag)
                 .with_assign_target(in_.assign_target)
-                .with_is_delete_target(is_delete_target);
+                .with_is_delete_target(is_delete_target)
+                .with_is_property_access_target(in_.is_property_access_target);
             if let Some(_expr) = p.maybe_rewrite_property_access(
                 expr.loc,
                 e_.target,

test/bundler/bundler_cjs.test.ts

@@ -373,10 +373,10 @@ describe("bundler", () => {
       `,
     },
     run: {
-      // The default import is `module.exports`, which for a lifted CommonJS
-      // module is the namespace object itself, so the namespace has no
-      // separate `default` key (the same as a lone `import *`, Test 6).
-      stdout: '{"default":{"foo":"foo","bar":"bar"},"named":"foo","namespace":{"foo":"foo","bar":"bar"}}',
+      // The default import is held as a value, so the module keeps its
+      // CommonJS wrapper and the namespace goes through __toESM, as in Node.
+      stdout:
+        '{"default":{"foo":"foo","bar":"bar"},"named":"foo","namespace":{"default":{"foo":"foo","bar":"bar"},"foo":"foo","bar":"bar"}}',
     },
   });