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

[Bug]: Can not read loaderContext.data when using pitch loader #6457

Closed
chenjiahan opened this issue May 7, 2024 · 4 comments · Fixed by #6753
Closed

[Bug]: Can not read loaderContext.data when using pitch loader #6457

chenjiahan opened this issue May 7, 2024 · 4 comments · Fixed by #6753
Assignees
Labels
bug Something isn't working pr welcome team The issue/pr is created by the member of Rspack.

Comments

@chenjiahan
Copy link
Member

System Info

System:
OS: macOS 13.6.6
CPU: (10) arm64 Apple M1 Pro
Memory: 2.56 GB / 32.00 GB
Shell: 5.9 - /bin/zsh
Binaries:
Yarn: 1.22.19 - ~/Library/Application Support/fnm/node-versions/v18.19.0/installation/bin/yarn
npm: 10.2.3 - ~/Library/Application Support/fnm/node-versions/v18.19.0/installation/bin/npm
pnpm: 7.18.1 - ~/Library/Application Support/fnm/node-versions/v18.19.0/installation/bin/pnpm
bun: 1.0.30 - ~/.bun/bin/bun
Browsers:
Chrome: 124.0.6367.119
Safari: 16.6
npmPackages:
@rspack/cli: ^0.6.3 => 0.6.3
@rspack/core: ^0.6.3 => 0.6.3

Details

Can not read loaderContext.data when using pitch loader.

For example, create a simple pitch loader and set loaderContext.data:

module.exports = function (code) {
  console.log("this.data", this.data);
  return code;
};

module.exports.pitch = function (_a, _b, data) {
  data.foo = "bar";
};
  • Webpack:
Screenshot 2024-05-07 at 11 29 18
  • Rspack:
Screenshot 2024-05-07 at 11 29 24

This seems to be a problem with the execution order of the pitch loaders in Rspack, causing data to not be set to the correct loader object.

Reproduce link

https://github.com/chenjiahan/rspack-repro-pitch-loader-data

Reproduce Steps

  1. pnpm i
  2. pnpm build:webpack
  3. pnpm build:rspack
@chenjiahan chenjiahan added bug Something isn't working pending triage The issue/PR is currently untouched. labels May 7, 2024
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label May 7, 2024
@h-a-n-a h-a-n-a self-assigned this May 8, 2024
@h-a-n-a
Copy link
Collaborator

h-a-n-a commented May 8, 2024

This is highly related with the current loader impl.

Normally, webpack loaders, in a single loader pipeline, share the same loader context with loaders in both pitching and normal stage. That is said the LoaderContext["data"] is shared as well. In rspack, after the pitching loader, the JavaScript loader runner will yield and give control back to Rust loader runner to load resources (loader runner's process_resource hook). This results in the data being lost.

@h-a-n-a h-a-n-a removed the pending triage The issue/PR is currently untouched. label May 8, 2024
@chenjiahan
Copy link
Member Author

Get it 👍

I found this issue when using the cache-loader, it is not urgent. If we can solve this issue, Rspack users can use cache-loader to speed up rebuild, which will be a practical approach before we support portable cache.

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented May 8, 2024

Yes, we have to resolve this issue. Roughly speaking, the rust LoaderItem(i.e. the LoaderObject counterpart in webpack) is immutable. To make it work with LoaderContext["data"], we need to change it to mutable and pass loader object with data instead of passing loader identifiers (upon which we create LoaderObject in loader runner on JS side) to JS to solve this issue.

@CPunisher
Copy link
Contributor

This also needs to dynamically add fields on data. Maybe we can refer to #6631 's implementation of saving data only on the js side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pr welcome team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants