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: fix pureDep returns null in some js files #16701

Merged
merged 1 commit into from Jan 4, 2024

Conversation

xiaoxiaojx
Copy link
Contributor

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
Not easy to reproduce, TODO

Does this PR introduce a breaking change?
no

What needs to be documented once your changes are merged?
no

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@xiaoxiaojx
Copy link
Contributor Author

xiaoxiaojx commented Feb 12, 2023

@alexander-akait @TheLarkInn @sokra This is a fatal error, please help to merge and release a version as soon as possible. For now I temporarily work around by disabling innerGraph

// webpack.config.js

module.exports = {
  //...
  optimization: {
+   innerGraph: false,
  },
};

By the way, another bugfix I submitted is best dealt with together, Thanks 🙏

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Sorry for delay, it looks good, but we need a test case here, can you add it?

@xiaoxiaojx
Copy link
Contributor Author

@alexander-akait I can't reproduce it in a simple demo, maybe I have to read more Inner-module tree-shaking related codes to write it. It would be appreciated if you could quickly complete the test case. Otherwise I might spend some time

@alexander-akait
Copy link
Member

@xiaoxiaojx it is not good to merge code without tests 😞 How you catch a such problem and how do you undestand that this fix helps?

@xiaoxiaojx
Copy link
Contributor Author

xiaoxiaojx commented Feb 18, 2023

@alexander-akait This problem occurred in a project of our company, So I can't upload this part of the reproduced code 😢 ,I've been trying to reproduce in a simple demo, but it always works 😅


What I found out is that a Module(29971) has a runtimes array(RuntimeSpec[])

{
  29971: function (t, e, r) {
    
    e.ZP =
      5224 == r.j
        ? function (t, e, r) {
            var i,
              o = e.payload.cacheKey;
            return n(n({}, t), (((i = {})[o] = m(t[o], e, r)), i));
          }
        : null;
  }
}
/** @typedef {string | SortableSet<string> | undefined} RuntimeSpec */

The value of this runtimes is [new SortableSet([5224, 5491, 1803]), new SortableSet([9768, 7327, 7069])]

But here only runtimes[0] is used when generating runtimeConditionExpression, the generated code is as follows

e.ZP = 5224 == r.j ? xxx : null

At this time, if 9768 in runtimes[1] also depends on this Module(29971), The export of Module(29971)is null ❌

So I changed the parameter passed in when generating runtimeConditionExpression from runtime to mergedRuntimes

-			const runtimeCondition = filterRuntime(runtime, runtime => {
+			const merged = deepMergeRuntime(runtimes, runtime);
+			const runtimeCondition = filterRuntime(merged, runtime => {

The code generated by Module (29971) becomes the following, My problem has also been solved ✅, because the js of 9768 Entrypoint can also get the export of Module (29971)

e.ZP = [5224, 9768].includes(r.j) ? xxx : null

@alexander-akait
Copy link
Member

Looks good for me, will be great to have a lot of @sokra here

@TheLarkInn TheLarkInn requested a review from sokra March 6, 2023 17:15
@TheLarkInn
Copy link
Member

Can you please add a tests case for this? Happy to help with this. If it's fixing something we should guard against these with tests for future changes.

@xiaoxiaojx
Copy link
Contributor Author

@TheLarkInn I tried hard to reproduce it in the demo, and then wrote test cases, but in the end I couldn't reproduce it. I think the Inner-module tree-shaking part of the code is complicated 😢 ......

@alexander-akait
Copy link
Member

@xiaoxiaojx maybe you can provide what do you try to solve with simpel examples of code, so I can try to help you with tests

@xiaoxiaojx
Copy link
Contributor Author

xiaoxiaojx commented Jun 7, 2023

@alexander-akait This problem occurred in a project of our company, So I can't upload this part of the reproduced code 😢 ,I've been trying to reproduce in a simple demo, but it always works 😅

What I found out is that a Module(29971) has a runtimes array(RuntimeSpec[])

{
  29971: function (t, e, r) {
    
    e.ZP =
      5224 == r.j
        ? function (t, e, r) {
            var i,
              o = e.payload.cacheKey;
            return n(n({}, t), (((i = {})[o] = m(t[o], e, r)), i));
          }
        : null;
  }
}
/** @typedef {string | SortableSet<string> | undefined} RuntimeSpec */

The value of this runtimes is [new SortableSet([5224, 5491, 1803]), new SortableSet([9768, 7327, 7069])]

But here only runtimes[0] is used when generating runtimeConditionExpression, the generated code is as follows

e.ZP = 5224 == r.j ? xxx : null

At this time, if 9768 in runtimes[1] also depends on this Module(29971), The export of Module(29971)is null ❌

So I changed the parameter passed in when generating runtimeConditionExpression from runtime to mergedRuntimes

-			const runtimeCondition = filterRuntime(runtime, runtime => {
+			const merged = deepMergeRuntime(runtimes, runtime);
+			const runtimeCondition = filterRuntime(merged, runtime => {

The code generated by Module (29971) becomes the following, My problem has also been solved ✅, because the js of 9768 Entrypoint can also get the export of Module (29971)

e.ZP = [5224, 9768].includes(r.j) ? xxx : null

@alexander-akait I'm not sure what conditions a Module has a runtimes array (RuntimeSpec[]). So far, no one else has reported this problem, and I have begun to wonder if this project uses a special plugin 🤷‍♂️. I can't provide more information at the moment, so let's put this pr on hold for now ...
❤️ Thanks for following up so far

plrthink pushed a commit to jinshuju/create-react-app that referenced this pull request Jul 23, 2023
@jdb8
Copy link
Contributor

jdb8 commented Oct 31, 2023

I just stumbled upon this issue after finding @xiaoxiaojx 's blog post pointing here (thank you!). We started seeing the mismatched module ID/null function export after switching from runtimeChunk: 'single' to runtimeChunk: true in a large multi-entrypoint repo under latest webpack 5.89.0.

The workaround to disable optimization.innerGraph seems to work, but unfortunately increases bundle sizes by quite a lot per-entrypoint (presumably due to benefits we'd otherwise gain from this optimisation). I'd rather not do that, so I'm wondering what the current status of this PR is?

I see there's some talk about tests, but my guess is that it might be hard to come up with a robust test here that captures the behaviour, since in our case we only saw this for one specific module in our monorepo out of thousands, and only after shipping a second, seemingly benign change (which presumably affected chunking/module concatenation/something else under the hood).

If I were to take a stab at the conditions that cause this, I'd say:

  • runtimeChunk: true
  • a named module export that's used across two separate entrypoints
  • importing and using the named export in the second entrypoint
  • ...more variables that are hard to track down

If there's more info I can provide that would help get this merged, that would be great.

@xiaoxiaojx
Copy link
Contributor Author

xiaoxiaojx commented Oct 31, 2023

Can confirm this is a bug. In this PR @plrthink commit/8b1b55b and @jdb8 also reported the same bug.

Currently pr can run through all test cases. It may be a good choice to merge it first. cc @alexander-akait @TheLarkInn

@alexander-akait
Copy link
Member

@xiaoxiaojx Hm, okay, our tests are success, so let's ship it, maybe in future we will add a test case here, sorry for delay

@xiaoxiaojx
Copy link
Contributor Author

@alexander-akait Thanks, looking forward to the new version !

@alexander-akait alexander-akait merged commit c1b45d5 into webpack:main Jan 4, 2024
52 of 56 checks passed
@xiaoxiaojx
Copy link
Contributor Author

xiaoxiaojx commented Feb 2, 2024

@plrthink @jdb8 webpack@5.90.0 has fixed,Let's re-enable this optimization,Thanks to everyone.

// webpack.config.js

module.exports = {
  //...
  optimization: {
+   innerGraph: true,
-   innerGraph: false,
  },
};

I verified that the webpack@5.90.0 is fine ✅
image

When webpack < 5.90.0 ❌
27bbbec5c3c6f95e54ae91eab2cbdb9a9af3a89f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Shipped
Development

Successfully merging this pull request may close these issues.

None yet

7 participants