Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort global deps before injecting imports #8818

Merged
merged 3 commits into from
Feb 7, 2023
Merged

Conversation

mattcompiles
Copy link
Contributor

@mattcompiles mattcompiles commented Feb 6, 2023

↪️ Pull Request

The global replacer currently injects imports to polyfills for node global APIs, however their order is not consistent between builds. This can be due to SWC not visiting the expressions in order or HashMap not storing values in insertion order.

The PR sorts globals by key before inserting them to ensure consistent ordering between builds.
This PR switches HashMap for IndexMap which respects insertion order.

🚨 Test instructions

Existing tests are sufficient.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@parcel-benchmark
Copy link

parcel-benchmark commented Feb 6, 2023

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.41s -1.00ms
Cached 331.00ms +2.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 89.00ms +5.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 90.00ms +5.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 9.19s -37.00ms
Cached 412.00ms -19.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.52m +786.00ms
Cached 2.23s +73.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 6.51s -60.00ms
Cached 264.00ms +4.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@mischnic
Copy link
Member

mischnic commented Feb 6, 2023

Haven't we tried to use indexedmap at some point for some part of the transformer but then went with arrays for some reason? Was it performance? @devongovett

--- packages/transformers/js/core/Cargo.toml
+++ packages/transformers/js/core/Cargo.toml
@@ -20,3 +20,4 @@ sha-1 = "0.10.0"
 dunce = "1.0.1"
 pathdiff = "0.2.0"
 path-slash = "0.1.4"
+indexmap = "1.9.2"
diff --git packages/transformers/js/core/src/global_replacer.rs packages/transformers/js/core/src/global_replacer.rs
index e249e6da2..80ed0a56a 100644
--- packages/transformers/js/core/src/global_replacer.rs
+++ packages/transformers/js/core/src/global_replacer.rs
@@ -1,7 +1,8 @@
 use path_slash::PathBufExt;
-use std::collections::{HashMap, HashSet};
+use std::collections::HashSet;
 use std::path::Path;
 
+use indexmap::IndexMap;
 use swc_atoms::JsWord;
 use swc_common::{Mark, SourceMap, SyntaxContext, DUMMY_SP};
 use swc_ecmascript::ast::{self, ComputedPropName, Id};
@@ -14,9 +15,10 @@ pub struct GlobalReplacer<'a> {
   pub source_map: &'a SourceMap,
   pub items: &'a mut Vec<DependencyDescriptor>,
   pub global_mark: Mark,
-  pub globals: HashMap<JsWord, (SyntaxContext, ast::Stmt)>,
+  pub globals: IndexMap<JsWord, (SyntaxContext, ast::Stmt)>,
   pub project_root: &'a Path,
   pub filename: &'a Path,
   pub decls: &'a mut HashSet<Id>,
   pub scope_hoist: bool,
 }
diff --git packages/transformers/js/core/src/lib.rs packages/transformers/js/core/src/lib.rs
index e0d51e61b..b8497eb33 100644
--- packages/transformers/js/core/src/lib.rs
+++ packages/transformers/js/core/src/lib.rs
@@ -22,6 +22,7 @@ mod node_replacer;
 mod typeof_replacer;
 mod utils;
 
+use indexmap::IndexMap;
 use std::collections::{HashMap, HashSet};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
@@ -369,7 +370,7 @@ pub fn transform(config: Config) -> Result<TransformResult, std::io::Error> {
                       source_map: &source_map,
                       items: &mut global_deps,
                       global_mark,
-                      globals: HashMap::new(),
+                      globals: IndexMap::new(),
                       project_root: Path::new(&config.project_root),
                       filename: Path::new(&config.filename),
                       decls: &mut decls,

@mattcompiles mattcompiles merged commit adb01fe into v2 Feb 7, 2023
@mattcompiles mattcompiles deleted the mjones/global-dep-order branch February 7, 2023 00:37
marcins pushed a commit to marcins/parcel that referenced this pull request Jul 14, 2023
* upstream/v2:
  Sort global deps before injecting imports (parcel-bundler#8818)
  Sort CSS module exports (parcel-bundler#8817)
  fix: add extra information to unique bundles (parcel-bundler#8784)
  Don't blow up HMR when <link />s don't have hrefs (parcel-bundler#8800)
lettertwo added a commit that referenced this pull request Nov 6, 2023
* upstream/v2: (128 commits)
  [webextension] Add support for `chrome_style` (#8867)
  Switch to SWC minifier by default (#8860)
  Use BitSet for bundler intersections (#8862)
  best key logic truncating package names (#8865)
  Add support for loadConfig to resolver plugins (#8847)
  Missing edge for multiple targets (#8854)
  Split large runtime manifest into separate bundles (#8837)
  Improvements to new resolver (#8844)
  Fix published files for resolver
  New resolver implementation in Rust (#8807)
  Update yarn.lock (#8843)
  Bump napi-rs to latest (#8838)
  Support .proxyrc.cjs  (#8833)
  Sort global deps before injecting imports (#8818)
  Sort CSS module exports (#8817)
  fix: add extra information to unique bundles (#8784)
  Don't blow up HMR when <link />s don't have hrefs (#8800)
  v2.8.3
  Changelog for v2.8.3
  Address bug by updating an asset reference and merge conditions (#8762)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants