Retain correct dependency order between imports and reexports without…

… scopehoisting (#8591)
parcel-bundler · Nov 4, 2022 · 932b6bd · 932b6bd
1 parent 27569c5
commit 932b6bd
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 83 deletions.
diff --git a/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/a.js b/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/a.js
@@ -0,0 +1,2 @@
+sideEffect("a");
+export const a = "a";
diff --git a/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/b.js b/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/b.js
@@ -0,0 +1,2 @@
+sideEffect("b");
+export const b = "b";
diff --git a/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/c.js b/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/c.js
@@ -0,0 +1,2 @@
+sideEffect("c");
+export const c = "c";
diff --git a/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/index.js b/packages/core/integration-tests/test/integration/js-import-reexport-dep-order/index.js
@@ -0,0 +1,5 @@
+export * from './a'
+
+export {b} from './b'
+
+import './c'
diff --git a/packages/core/integration-tests/test/javascript.js b/packages/core/integration-tests/test/javascript.js
@@ -5302,6 +5302,20 @@ describe('javascript', function () {
     assert.deepEqual(res.default, ['a', 'b', 'bar']);
   });
 
+  it('should retain the correct dependency order between import and reexports', async function () {
+    let b = await bundle(
+      path.join(__dirname, 'integration/js-import-reexport-dep-order/index.js'),
+    );
+
+    let calls = [];
+    await run(b, {
+      sideEffect(v) {
+        calls.push(v);
+      },
+    });
+    assert.deepEqual(calls, ['a', 'b', 'c']);
+  });
+
   it('should not freeze live default imports', async function () {
     let b = await bundle(
       path.join(__dirname, 'integration/js-import-default-live/index.js'),

diff --git a/packages/transformers/js/core/src/modules.rs b/packages/transformers/js/core/src/modules.rs
@@ -264,43 +264,110 @@ macro_rules! modules_visit_fn {
 impl Fold for ESMFold {
   fn fold_module(&mut self, node: Module) -> Module {
     let mut is_esm = false;
+    let mut needs_interop_flag = false;
 
-    // First pass: collect all imported declarations.
+    // First pass: collect all imported declarations. On the second pass, exports can be matched to
+    // imports (to better handle import/export pairs that are really just reexports).
+    //
+    // To ensure that all declarations that cause dependencies are kept in the same order, handle
+    // export declarations with a source in the first pass as well.
     for item in &node.body {
       if let ModuleItem::ModuleDecl(decl) = &item {
         is_esm = true;
-        if let ModuleDecl::Import(import) = decl {
-          self.create_require(import.src.value.clone(), import.span);
-
-          for specifier in &import.specifiers {
-            match specifier {
-              ImportSpecifier::Named(named) => {
-                let imported = match &named.imported {
-                  Some(imported) => match_export_name(imported).0.clone(),
-                  None => named.local.sym.clone(),
-                };
-                self.imports.insert(
-                  id!(named.local),
-                  (import.src.value.clone(), imported.clone()),
-                );
-                if imported == js_word!("default") {
+        match decl {
+          ModuleDecl::Import(import) => {
+            self.create_require(import.src.value.clone(), import.span);
+
+            for specifier in &import.specifiers {
+              match specifier {
+                ImportSpecifier::Named(named) => {
+                  let imported = match &named.imported {
+                    Some(imported) => match_export_name(imported).0.clone(),
+                    None => named.local.sym.clone(),
+                  };
+                  self.imports.insert(
+                    id!(named.local),
+                    (import.src.value.clone(), imported.clone()),
+                  );
+                  if imported == js_word!("default") {
+                    self.create_interop_default(import.src.value.clone());
+                  }
+                }
+                ImportSpecifier::Default(default) => {
+                  self.imports.insert(
+                    id!(default.local),
+                    (import.src.value.clone(), "default".into()),
+                  );
                   self.create_interop_default(import.src.value.clone());
                 }
+                ImportSpecifier::Namespace(namespace) => {
+                  self
+                    .imports
+                    .insert(id!(namespace.local), (import.src.value.clone(), "*".into()));
+                }
               }
-              ImportSpecifier::Default(default) => {
-                self.imports.insert(
-                  id!(default.local),
-                  (import.src.value.clone(), "default".into()),
-                );
-                self.create_interop_default(import.src.value.clone());
-              }
-              ImportSpecifier::Namespace(namespace) => {
-                self
-                  .imports
-                  .insert(id!(namespace.local), (import.src.value.clone(), "*".into()));
+            }
+          }
+          ModuleDecl::ExportNamed(export) => {
+            needs_interop_flag = true;
+
+            if let Some(src) = &export.src {
+              self.create_require(src.value.clone(), export.span);
+
+              for specifier in &export.specifiers {
+                match specifier {
+                  ExportSpecifier::Named(named) => {
+                    let exported = match &named.exported {
+                      Some(exported) => exported.clone(),
+                      None => named.orig.clone(),
+                    };
+
+                    if match_export_name(&named.orig).0 == js_word!("default") {
+                      self.create_interop_default(src.value.clone());
+                    }
+
+                    let specifier = self.create_import_access(
+                      &src.value,
+                      &match_export_name(&named.orig).0,
+                      DUMMY_SP,
+                    );
+                    self.create_export(match_export_name(&exported).0, specifier, export.span);
+                  }
+                  ExportSpecifier::Default(default) => {
+                    self.create_interop_default(src.value.clone());
+                    let specifier =
+                      self.create_import_access(&src.value, &js_word!("default"), DUMMY_SP);
+                    self.create_export(default.exported.sym.clone(), specifier, export.span);
+                  }
+                  ExportSpecifier::Namespace(namespace) => {
+                    let local = self.get_require_name(&src.value, DUMMY_SP);
+                    self.create_export(
+                      match_export_name(&namespace.name).0,
+                      Expr::Ident(local),
+                      export.span,
+                    )
+                  }
+                }
               }
+            } else {
+              // Handled below
             }
           }
+          ModuleDecl::ExportAll(export) => {
+            needs_interop_flag = true;
+            self.create_require(export.src.value.clone(), export.span);
+            let require_name = self.get_require_name(&export.src.value, export.span);
+            let export = self.call_helper(
+              "exportAll".into(),
+              vec![
+                Expr::Ident(require_name),
+                Expr::Ident(Ident::new("exports".into(), DUMMY_SP)),
+              ],
+              export.span,
+            );
+            self.requires.push(export);
+          }
+          _ => (),
         }
       }
     }
@@ -311,58 +378,19 @@ impl Fold for ESMFold {
     }
 
     let node = node.fold_children_with(self);
-    let mut needs_interop_flag = false;
     let mut items = vec![];
 
+    // Second pass
     for item in &node.body {
       match &item {
         ModuleItem::ModuleDecl(decl) => {
           match decl {
-            ModuleDecl::Import(_import) => {
+            ModuleDecl::Import(_) | ModuleDecl::ExportAll(_) => {
               // Handled above
             }
             ModuleDecl::ExportNamed(export) => {
               needs_interop_flag = true;
-
-              if let Some(src) = &export.src {
-                self.create_require(src.value.clone(), export.span);
-
-                for specifier in &export.specifiers {
-                  match specifier {
-                    ExportSpecifier::Named(named) => {
-                      let exported = match &named.exported {
-                        Some(exported) => exported.clone(),
-                        None => named.orig.clone(),
-                      };
-
-                      if match_export_name(&named.orig).0 == js_word!("default") {
-                        self.create_interop_default(src.value.clone());
-                      }
-
-                      let specifier = self.create_import_access(
-                        &src.value,
-                        &match_export_name(&named.orig).0,
-                        DUMMY_SP,
-                      );
-                      self.create_export(match_export_name(&exported).0, specifier, export.span);
-                    }
-                    ExportSpecifier::Default(default) => {
-                      self.create_interop_default(src.value.clone());
-                      let specifier =
-                        self.create_import_access(&src.value, &js_word!("default"), DUMMY_SP);
-                      self.create_export(default.exported.sym.clone(), specifier, export.span);
-                    }
-                    ExportSpecifier::Namespace(namespace) => {
-                      let local = self.get_require_name(&src.value, DUMMY_SP);
-                      self.create_export(
-                        match_export_name(&namespace.name).0,
-                        Expr::Ident(local),
-                        export.span,
-                      )
-                    }
-                  }
-                }
-              } else {
+              if export.src.is_none() {
                 for specifier in &export.specifiers {
                   if let ExportSpecifier::Named(named) = specifier {
                     let exported = match &named.exported {
@@ -386,22 +414,10 @@ impl Fold for ESMFold {
                     self.create_export(match_export_name(&exported).0, value, export.span);
                   }
                 }
+              } else {
+                // Handled above
               }
             }
-            ModuleDecl::ExportAll(export) => {
-              needs_interop_flag = true;
-              self.create_require(export.src.value.clone(), export.span);
-              let require_name = self.get_require_name(&export.src.value, export.span);
-              let export = self.call_helper(
-                "exportAll".into(),
-                vec![
-                  Expr::Ident(require_name),
-                  Expr::Ident(Ident::new("exports".into(), DUMMY_SP)),
-                ],
-                export.span,
-              );
-              self.requires.push(export);
-            }
             ModuleDecl::ExportDefaultExpr(export) => {
               needs_interop_flag = true;
               items.push(self.create_exports_assign(