Skip to content

feat: consider asset module as side-effect-free#20352

Merged
alexander-akait merged 8 commits into
mainfrom
asset-side-effect-free
Feb 2, 2026
Merged

feat: consider asset module as side-effect-free#20352
alexander-akait merged 8 commits into
mainfrom
asset-side-effect-free

Conversation

@hai-x

@hai-x hai-x commented Jan 24, 2026

Copy link
Copy Markdown
Member

Summary

What kind of change does this PR introduce?

Fixes part of #20209.

Did you add tests for your changes?

Yes

Does this PR introduce a breaking change?

Theoretically not

If relevant, what needs to be documented once your changes are merged or what have you already documented?

No

@changeset-bot

changeset-bot Bot commented Jan 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 288c500

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
webpack Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Jan 24, 2026

Copy link
Copy Markdown
Contributor

This PR is packaged and the instant preview is available (5ecc58d).

Install it locally:

  • npm
npm i -D webpack@https://pkg.pr.new/webpack@5ecc58d
  • yarn
yarn add -D webpack@https://pkg.pr.new/webpack@5ecc58d
  • pnpm
pnpm add -D webpack@https://pkg.pr.new/webpack@5ecc58d

@hai-x hai-x force-pushed the asset-side-effect-free branch from 2bed3ee to c56b046 Compare January 24, 2026 12:37
@codspeed-hq

codspeed-hq Bot commented Jan 24, 2026

Copy link
Copy Markdown

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing asset-side-effect-free (288c500) with main (fe48655)1

Summary

✅ 72 untouched benchmarks

Footnotes

  1. No successful run was found on main (cd4793d) during the generation of this report, so fe48655 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@alexander-akait

Copy link
Copy Markdown
Member

Looks like we need to fix tests, but code looks good

@hai-x

hai-x commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

We should consider a better approach for setting factoryMeta. It is assigned at parsing stage right now, it will lead to some issue when working with both disk and memory caching, and cause module.factoryMeta can't be properly restored.

@alexander-akait

Copy link
Copy Markdown
Member

@hai-x Yeah, we have the same problem with renaming modules, alternative solution - make builtMeta and buildInfo as getters, but will be glad to have a hook to setup it

@hai-x

hai-x commented Jan 29, 2026

Copy link
Copy Markdown
Member Author

I use createModuleClass here cause it will be called at the factory stage. And it also aligns with our goal of creating asset modules via newly AssetModule(it's still TODO).

@hai-x hai-x marked this pull request as ready for review January 29, 2026 18:16
})
);
}
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about to rewrite test to see that unused module was removed, we have __webpack_modules__ to check them in chunk

@hai-x hai-x force-pushed the asset-side-effect-free branch from 3302b9d to 54d2801 Compare February 1, 2026 17:07
@hai-x hai-x force-pushed the asset-side-effect-free branch from 4d2a9c1 to c3b05b6 Compare February 1, 2026 17:14
@hai-x

hai-x commented Feb 1, 2026

Copy link
Copy Markdown
Member Author

Thanks for review. It such one good idea to use __webpack_modules__.

@alexander-akait alexander-akait merged commit 5ecc58d into main Feb 2, 2026
49 of 52 checks passed
@alexander-akait alexander-akait deleted the asset-side-effect-free branch February 2, 2026 16:30
kleisauke added a commit to kleisauke/wasm-vips that referenced this pull request Feb 23, 2026
Webpack v5.105.0 treats assets as side-effect free, see:
webpack/webpack#20352
@GerkinDev

Copy link
Copy Markdown

Just a heads up: this broke our build. We marked fonts to include in the build with side-effect only imports from our code

import "~/assets/my-font.woff"

and as expected, this caused the font not to be copied anymore.

I think there should be extra documentation in https://webpack.js.org/guides/asset-modules mentionning this can be forced using

      {
        test: /\.(woff|woff2)$/,
        generator: {
          filename: 'fonts/[name][ext]',
        },
        type: 'asset/resource',
        sideEffects: true,
      },

@alexander-akait

Copy link
Copy Markdown
Member

@GerkinDev We should fix it, it is a bug

/cc @hai-x Let's do mark them as side effect free for side-effect only imports

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