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

Fix Best key logic truncating package names #8865

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented Mar 2, 2023

↪️ Pull Request

Logic below allowed for incorrect matching of path to specifier when resolving.

Context: Encountered an issue with the resolver in an app where package names were split as a result of variable bestKey always passing the first loop condition for altering longest_suffix , resulting in path splitting a specifier that did not include the path, or worse, and out of bounds error on a later splitting.

💻 Examples

Not sure exactly how to test this, but this is the situation I ran into / highlights the logic issue

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=26d018c121654e1beb80a45c77bfd991

✔️ PR Todo

  • Added/updated unit tests for this change

@AGawrys AGawrys requested a review from devongovett March 2, 2023 01:46
@parcel-benchmark
Copy link

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 2.35s +34.00ms
Cached 487.00ms +28.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description Time Difference
Cold 15.34s -198.00ms
Cached 595.00ms +19.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 2.82m +1.07s
Cached 2.92s -58.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 12.18s -254.00ms
Cached 371.00ms -27.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Classic. Thanks!

@devongovett devongovett merged commit 28a2744 into v2 Mar 2, 2023
@devongovett devongovett deleted the resolver-rs-tsconfig-outofbounds-fix branch March 2, 2023 15:26
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.

3 participants