fix: passively rebuild modules imported by importModule

[CPunisher]

Summary

Investigation: #7805 (comment) and #7805 (comment)

Do not rebuild B ahead. Instead, let A introduce B again.

Changes: The executor doesn't rebuilt the referenced modules but only updates the module graph. Since the referencing module also contains file dependecies of the referenced module, the importModule will be called again and the referenced module is rebuilt. Finally we fix the artifact after finishing all modules.

closes #7805
closes #8208

Checklist

• Tests updated (or not required).
• Documentation updated (or not required).

[netlify]

✅ Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	d973cf8
🔍 Latest deploy log	https://app.netlify.com/sites/rspack/deploys/6751a325963e650008f2f673

[jerrykingxyz]

This process does not seem to be any different from the current process. I guess this error might be caused by the finish_check in the ctrl task. @CPunisher Are you interested in refactoring the finish_check logic? I can provide context and solutions.

[CPunisher]

This process does not seem to be any different from the current process. I guess this error might be caused by the finish_check in the ctrl task. @CPunisher Are you interested in refactoring the finish_check logic? I can provide context and solutions.

They are different. Let's assume one of the loaders of module A import module B by this.importModule.
With original process, when the file dependencies of module B change, module B is rebuilt before module A in hook_before_make by the executor, and then module A is rebuilt in make.
With this changed process, when the file dependencies of module B change, the executor only update the module graph(cutoff), module B is not rebuilt util the module A is rebuilding and its loader call this.importModule(moduleB) again.
From my test, the issue can be fixed by this pr.

If you think it's not the root cause or it's not appropriate, I'm also glad to help refactor the anything.

[jerrykingxyz]

emmm...Let me review current logic.

1. we have a moduleA and use this.importModule to import moduleB, the moduleA will be added to compilation.module_graph, and the moduleB will be added to module_executor.module_graph.

2. modify the file which moduleB depends on. The moduleA will rebuild because its file_dependencies include all of moduleB’s file dependencies.

3. when make start, the module_executor will remove moduleB from module_executor.module_graph by cutout, and will rebuild moduleB by repair. both cutoff and repair are wrapped in update_module_graph. https://github.com/web-infra-dev/rspack/blob/main/crates/rspack_core/src/compiler/module_executor/mod.rs#L68

4. make will cutout the moduleA and rebuild it. The moduleA will import the module again using this.importModule. The module_executor will run ExecuteTask directly, because module_executor.hook_before_make will ensure that module_executor.module_graph is updated.

From the above process, I think this PR just postpones the timing of rebuilding moduleB. @CPunisher It would be more helpful if you could add some test cases for this case.

[jerrykingxyz]

@CPunisher Good edge case, but this PR will introduce other bugs. Lets see a example:

1. moduleA use importModule to add moduleB, and moduleB needs to use moduleC, we will get a module_executor.module_graph contains moduleB and moduleC, moduleB depends on moduleC.

2. update the file that moduleC depends on. The moduleC will be remove and never regenerate in this PR, because of only catcut is called and not repair.

I have written a test case for it. f782685

Look back to the issue that this PR will fixed, I think we can fix it by just remove the dependency at request_dep_map from the cutout result.

1. add a new MakeParam to skip dependencies.

pub enum MakeParam {
   ...,
   ForceSkipDeps(HashSet<DependencyId>)
}

2. impl skip logic at cutout

pub fn cutout_artifact() {
    let skip_deps = HashSet::default();
    ...
    for item in params {
      match item {
          MakeParam::ForceSkipDeps(deps) => {
              skip_deps.extend(deps);
          }
      }
    }
    ...
    force_build_deps.filter(|item| !skip_deps.includes(item))
}

3. module_executor call update_module_graph with MakeParam::ForceSkipDeps

update_module_graph(vec![..., MakeParam::ForceSkipDeps(self.request_dep_map.values())])

[CPunisher]

@jerrykingxyz I agree with your idea. I will try it.

[CPunisher]

@jerrykingxyz OK, after trying I find that we have to merge our ideas. Let's continue your example. We should consider three cases.

1. Only moduleB updates. We don't rebuild moduleB in hook_before_make, but revoke it in the graph.
2. Only moduleC updates. We should rebuild moduleC in hook_before_make because we can make sure that moduleC is still referenced by moduleB.
3. Both moduleB and moduleC update. We don't rebuild moduleB in hook_before_make. We can either rebuild moduleC or just revoke it. Although the latter is more efficient but cumbersome to implement, I want to keep everything simple now.

[jerrykingxyz]

For case 3, the build_dependencies returned by cutout does not include the dependencies for building ModuleC. Therefore, your code is the same as defining a new MakeParam.

The follow code will remove the dependencies to build ModuleC.
https://github.com/web-infra-dev/rspack/blob/main/crates/rspack_core/src/compiler/make/cutout/mod.rs#L131-L141

[CPunisher]

For case 3, the build_dependencies returned by cutout does not include the dependencies for building ModuleC. Therefore, your code is the same as defining a new MakeParam.

The follow code will remove the dependencies to build ModuleC. https://github.com/web-infra-dev/rspack/blob/main/crates/rspack_core/src/compiler/make/cutout/mod.rs#L131-L141

Only defining a new MakeParam and skip rebuilding moduleB is not enough. We also need to get build_dependencies to determine whether we should remove it (or add a should_update mark as I do now) from the request_dep_map when it is revoked. So the moduleA will import moduleB again.

Based on above, I give up the new MakeParam since we can directly mutate the build_dependencies collection.

[jerrykingxyz]

LGTM. This fix is ​​a good one for now. I think we'll have better options once we refactor the "finish_check" logic.

[jerrykingxyz]

@CPunisher Thanks for fixing this. I'll create a new issue to provide context about the finish_check refactoring if you're interested.