Skip to content

Conversation

@OlenDavis
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

1. Performance refactor / race-condition fix:

This PR eliminates the race condition where the loader or optimizer may do the same processing (same transformation on the same image) redundantly if the same file transformation's loader is triggered more than once before the first transformation has finished.

2. Fix npm start

Didn't work before; now it does. (With as little change to the prior scripts as possible.)

Breaking Changes

None that I'm aware of.

Additional Info

This PR doesn't eliminate all possible such race conditions, namely if the same image transformation occurs between the loader and the minimizer, because the transformers are different functions (because the minimizer functionality is different than the generator/loader, even though the underlying operation is the same), it's still possible to have redundant image transformations if one transformation happens via the loader, and the same transformation happens via the webpack optimize hook.

@codecov
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.15%. Comparing base (3a924e4) to head (b738fa4).
Report is 31 commits behind head on master.

Files with missing lines Patch % Lines
src/loader.js 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   77.16%   78.15%   +0.98%     
==========================================
  Files           4        4              
  Lines         727      769      +42     
  Branches      282      304      +22     
==========================================
+ Hits          561      601      +40     
- Misses        137      139       +2     
  Partials       29       29              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

logger.debug(`loader cache miss: ${filename}`);

return worker(minifyOptions);
});
Copy link
Member

Choose a reason for hiding this comment

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

We can't do it in loader:

  1. this._compilation.getCache is unavaliable in multi threading mode (thread-loader)
  2. Also this plugin is using by rspack too and such api is not working there

This PR eliminates the race condition where the loader or optimizer may do the same processing (same transformation on the same image) redundantly if the same file transformation's loader is triggered more than once before the first transformation has finished.

Can you provide reproducible example?

Also I think we can use WeakMap in the loader to prevent such situations

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.

2 participants