bundler: read destructured require() of an unwrapped package from its import namespace

src/ast/e.rs

@@ -2401,7 +2401,7 @@ pub type UnwrappedRequireIndexOptional =
 pub struct RequireString {
     pub import_record_index: u32,
 
-    /// Set when `unwrap_commonjs_to_esm` turned this `require()` into an import.
+    /// Set when unwrapping `const x = require()` into an import; consumed by `visit_decls`.
     pub unwrapped_id: UnwrappedRequireIndexOptional,
 }
 impl Default for RequireString {

src/bundler/LinkerContext.rs

@@ -2309,25 +2309,16 @@ impl<'a> LinkerContext<'a> {
     pub(crate) fn require_or_import_meta_for_source(
         &mut self,
         source_index: crate::IndexInt,
-        was_unwrapped_require: bool,
     ) -> js_printer::RequireOrImportMeta {
         let flags = self.graph.meta.items_flags()[source_index as usize];
         js_printer::RequireOrImportMeta {
-            exports_ref: if flags.wrap == WrapKind::Esm
-                || (was_unwrapped_require
-                    && self.graph.ast.items_flags()[source_index as usize]
-                        .contains(AstFlags::FORCE_CJS_TO_ESM))
-            {
+            exports_ref: if flags.wrap == WrapKind::Esm {
                 self.graph.ast.items_exports_ref()[source_index as usize]
             } else {
                 Ref::NONE
             },
             is_wrapper_async: flags.is_async_or_has_async_dependency,
             wrapper_ref: self.graph.ast.items_wrapper_ref()[source_index as usize],
-
-            was_unwrapped_require: was_unwrapped_require
-                && self.graph.ast.items_flags()[source_index as usize]
-                    .contains(AstFlags::FORCE_CJS_TO_ESM),
         }
     }
 
@@ -2562,12 +2553,8 @@ impl<'a> LinkerContext<'a> {
 /// can call back into `LinkerContext::require_or_import_meta_for_source`.
 impl<'a> js_printer::RequireOrImportMetaSource for LinkerContext<'a> {
     #[inline]
-    fn require_or_import_meta_for_source(
-        &mut self,
-        id: u32,
-        was_unwrapped_require: bool,
-    ) -> js_printer::RequireOrImportMeta {
-        LinkerContext::require_or_import_meta_for_source(self, id, was_unwrapped_require)
+    fn require_or_import_meta_for_source(&mut self, id: u32) -> js_printer::RequireOrImportMeta {
+        LinkerContext::require_or_import_meta_for_source(self, id)
     }
 }
 

src/js_parser/parser.rs

@@ -1011,8 +1011,7 @@ pub struct ExprIn {
     /// tests.
     pub(crate) assign_target: js_ast::AssignTarget,
 
-    /// Currently this is only used when unwrapping a call to `require()`
-    /// with `__toESM()`.
+    /// Identifier-binding initializer; only used to unwrap `const x = require()` into an import.
     pub(crate) is_immediately_assigned_to_decl: bool,
 
     pub(crate) property_access_for_method_call_maybe_should_replace_with_undefined: bool,

src/js_parser/visit/mod.rs

@@ -320,7 +320,11 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 self.visit_expr_in_out(
                     &mut val,
                     ExprIn {
-                        is_immediately_assigned_to_decl: true,
+                        // Only the identifier unwrap below consumes the marker this requests.
+                        is_immediately_assigned_to_decl: matches!(
+                            decl.binding.data,
+                            BData::BIdentifier(_)
+                        ),
                         ..Default::default()
                     },
                 );

src/js_printer/lib.rs

@@ -1194,20 +1194,15 @@ pub struct Options<'a> {
 }
 
 impl<'a> Options<'a> {
-    pub(crate) fn require_or_import_meta_for_source(
-        &self,
-        id: u32,
-        was_unwrapped_require: bool,
-    ) -> RequireOrImportMeta {
+    pub(crate) fn require_or_import_meta_for_source(&self, id: u32) -> RequireOrImportMeta {
         if self
             .require_or_import_meta_for_source_callback
             .ctx
             .is_none()
         {
             return RequireOrImportMeta::default();
         }
-        self.require_or_import_meta_for_source_callback
-            .call(id, was_unwrapped_require)
+        self.require_or_import_meta_for_source_callback.call(id)
     }
 }
 
@@ -1281,20 +1276,19 @@ pub struct RequireOrImportMeta {
     pub wrapper_ref: Ref,
     pub exports_ref: Ref,
     pub is_wrapper_async: bool,
-    pub was_unwrapped_require: bool,
 }
 
 // Clone/Copy: bitwise OK — `ctx` is a non-owning opaque backref the caller
 // keeps alive for the print pass; `callback` is POD.
 #[derive(Clone, Copy)]
 pub struct RequireOrImportMetaCallback {
     pub(crate) ctx: Option<NonNull<()>>,
-    pub(crate) callback: fn(*mut (), u32, bool) -> RequireOrImportMeta,
+    pub(crate) callback: fn(*mut (), u32) -> RequireOrImportMeta,
 }
 
 impl Default for RequireOrImportMetaCallback {
     fn default() -> Self {
-        fn noop(_: *mut (), _: u32, _: bool) -> RequireOrImportMeta {
+        fn noop(_: *mut (), _: u32) -> RequireOrImportMeta {
             RequireOrImportMeta::default()
         }
         Self {
@@ -1307,28 +1301,20 @@ impl Default for RequireOrImportMetaCallback {
 /// PORTING.md §Dispatch — manual vtable. The erased thunk is monomorphized
 /// over `T: RequireOrImportMetaSource`, so `callback` stays a captureless `fn`.
 pub trait RequireOrImportMetaSource {
-    fn require_or_import_meta_for_source(
-        &mut self,
-        id: u32,
-        was_unwrapped_require: bool,
-    ) -> RequireOrImportMeta;
+    fn require_or_import_meta_for_source(&mut self, id: u32) -> RequireOrImportMeta;
 }
 
 impl RequireOrImportMetaCallback {
-    pub(crate) fn call(&self, id: u32, was_unwrapped_require: bool) -> RequireOrImportMeta {
-        (self.callback)(self.ctx.unwrap().as_ptr(), id, was_unwrapped_require)
+    pub(crate) fn call(&self, id: u32) -> RequireOrImportMeta {
+        (self.callback)(self.ctx.unwrap().as_ptr(), id)
     }
 
     pub fn init<T: RequireOrImportMetaSource>(ctx: &mut T) -> Self {
-        fn thunk<T: RequireOrImportMetaSource>(
-            p: *mut (),
-            id: u32,
-            was_unwrapped_require: bool,
-        ) -> RequireOrImportMeta {
+        fn thunk<T: RequireOrImportMetaSource>(p: *mut (), id: u32) -> RequireOrImportMeta {
             // SAFETY: `p` was constructed from `&mut T` in `init` below; caller guarantees
             // `ctx` outlives this `RequireOrImportMetaCallback`, so the cast-back
             // deref is valid and exclusive.
-            unsafe { (*p.cast::<T>()).require_or_import_meta_for_source(id, was_unwrapped_require) }
+            unsafe { (*p.cast::<T>()).require_or_import_meta_for_source(id) }
         }
         Self {
             // Type-erased to `*mut ()` and cast back to `*mut T` inside the thunk before dereference.
@@ -1822,7 +1808,6 @@ pub(crate) mod __gated_printer {
                 match statement {
                     None => self.print_require_or_import_expr(
                         import.import_record_index,
-                        false,
                         &[],
                         Expr::EMPTY,
                         Level::Lowest,
@@ -1843,7 +1828,6 @@ pub(crate) mod __gated_printer {
                         self.print_equals();
                         self.print_require_or_import_expr(
                             import.import_record_index,
-                            false,
                             &[],
                             Expr::EMPTY,
                             Level::Lowest,
@@ -1893,7 +1877,6 @@ pub(crate) mod __gated_printer {
                     match statement {
                         None => self.print_require_or_import_expr(
                             import.import_record_index,
-                            false,
                             &[],
                             Expr::EMPTY,
                             Level::Lowest,
@@ -2455,7 +2438,6 @@ pub(crate) mod __gated_printer {
         pub(crate) fn print_require_or_import_expr(
             &mut self,
             import_record_index: u32,
-            was_unwrapped_require: bool,
             leading_interior_comments: &[G::Comment],
             import_options: Expr,
             level_: Level,
@@ -2504,10 +2486,9 @@ pub(crate) mod __gated_printer {
             }
 
             if record.source_index.is_valid() {
-                let mut meta = self.options.require_or_import_meta_for_source(
-                    record.source_index.get(),
-                    was_unwrapped_require,
-                );
+                let mut meta = self
+                    .options
+                    .require_or_import_meta_for_source(record.source_index.get());
 
                 // Don't need the namespace object if the result is unused anyway
                 if flags.contains(ExprFlag::ExprResultIsUnused) {
@@ -2534,7 +2515,6 @@ pub(crate) mod __gated_printer {
                 // Internal "require()" or "import()"
                 let has_side_effects = meta.wrapper_ref.is_valid()
                     || meta.exports_ref.is_valid()
-                    || meta.was_unwrapped_require
                     || self.options.input_files_for_dev_server.is_some();
                 if record.kind == ImportKind::Dynamic {
                     self.print_space_before_identifier();
@@ -2568,7 +2548,7 @@ pub(crate) mod __gated_printer {
                     let path = &input_files[record.source_index.get() as usize].path;
                     self.print_string_literal_utf8(path.pretty, false);
                     self.print(b")");
-                } else if !meta.was_unwrapped_require {
+                } else {
                     // Call the wrapper
                     if meta.wrapper_ref.is_valid() {
                         self.print_space_before_identifier();
@@ -2596,10 +2576,6 @@ pub(crate) mod __gated_printer {
                             self.print(b")");
                         }
                     }
-                } else {
-                    if !meta.exports_ref.is_empty() {
-                        self.print_symbol(meta.exports_ref);
-                    }
                 }
 
                 if wrap_with_to_esm {
@@ -3313,9 +3289,10 @@ pub(crate) mod __gated_printer {
                     }
                 }
                 ExprData::ERequireString(e) => {
+                    // An unwrapped require() is consumed by the parser's visit_decls, never printed.
+                    debug_assert!(e.unwrapped_id.is_none());
                     self.print_require_or_import_expr(
                         e.import_record_index,
-                        e.unwrapped_id.is_some(),
                         &[],
                         Expr::EMPTY,
                         level,
@@ -3383,7 +3360,6 @@ pub(crate) mod __gated_printer {
                     } else {
                         self.print_require_or_import_expr(
                             e.import_record_index,
-                            false,
                             &[], // e.leading_interior_comments,
                             e.options,
                             level,

test/bundler/bundler_cjs2esm.test.ts

@@ -284,15 +284,70 @@ describe("bundler", () => {
       stdout: "react\nreact\nreact\nreact\nundefined\nreact\nreact\nreact\nreact\nreact\nreact\n1 react\nreact\nreact",
     },
   });
-  // A require() of an unwrapped package that initializes a destructuring
-  // declaration is kept as a require expression that remembers it was
-  // unwrapped, and prints as the namespace object. One inside try/catch is
+  // The required packages assign module.exports, so they stay wrapped in __commonJS.
+  // A destructuring declaration has to read from the import the require() became,
+  // not from the wrapped module's own `exports` binding.
+  itBundled("cjs2esm/UnwrappedModuleRequireDestructured", {
+    files: {
+      "/entry.js": /* js */ `
+        const { react } = require("react");
+        console.log(react);
+
+        let { react: renamed, missing = "fallback" } = require("react");
+        console.log(renamed, missing);
+
+        var { react: { length } } = require("react");
+        console.log(length);
+
+        const before = require("react"),
+          { react: between } = require("react"),
+          after = require("react");
+        console.log(before.react, between, after.react);
+
+        function inFunction() {
+          const { react } = require("react");
+          return react;
+        }
+        console.log(inFunction());
+
+        const [first, second] = require("scheduler");
+        console.log(first, second);
+      `,
+      ...fakeReactNodeModules,
+      "/node_modules/scheduler/index.js": /* js */ `
+        module.exports = ["first", "second"];
+      `,
+      "/node_modules/scheduler/package.json": /* json */ `
+        {
+          "name": "scheduler",
+          "version": "1.0.0",
+          "main": "index.js"
+        }
+      `,
+    },
+    onAfterBundle: api => {
+      const code = api.readFile("out.js");
+      expect(code).toContain("var require_react = __commonJS(");
+      expect(code).toContain("var require_scheduler = __commonJS(");
+      expect(code).toMatch(/\{ react: between \} = \w+;/);
+      expect(code).toMatch(/\[first, second\] = \w+;/);
+    },
+    run: {
+      stdout: "react\nreact fallback\n5\nreact react react\nreact\nfirst second",
+    },
+  });
+  // Against a package that does convert to ESM, a destructuring declaration
+  // reads from the generated namespace object. A require() inside try/catch is
   // never unwrapped and prints as an ordinary require.
   itBundled("cjs2esm/UnwrappedModuleRequireDestructuredAndInTry", {
     files: {
       "/entry.js": /* js */ `
-        const { react: named } = require("react");
-        console.log(named);
+        const { react: named, version = "none" } = require("react");
+        console.log(named, version);
+
+        const whole = require("react"),
+          { react: again } = require("react");
+        console.log(whole.react, again);
 
         let inTry = "missing";
         try {
@@ -313,11 +368,12 @@ describe("bundler", () => {
     },
     onAfterBundle: api => {
       const code = api.readFile("out.js");
-      expect(code).toMatch(/\{ react: named \} = \(?exports_react\)?;/);
+      expect(code).toMatch(/\{ react: named, version = "none" \} = \(?exports_react\)?;/);
+      expect(code).toMatch(/\{ react: again \} = \(?exports_react\)?;/);
       expect(code).toContain("__toCommonJS(exports_react)).react");
     },
     run: {
-      stdout: "react\nreact",
+      stdout: "react none\nreact react\nreact",
     },
   });
   itBundled("cjs2esm/ReactSpecificUnwrapping", {