DefinePlugin can cause a ModuleConcatenationPlugin bug

[pbarbiero]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
ModuleConcatenationPlugin is moving imports needed at runtime to a function with delayed execution

If the current behavior is a bug, please provide the steps to reproduce.
Very difficult, we have a (very large) specific application that only recently exposed this once it reached a certain size.

What is the expected behavior?
ModuleConcatenationPlugin leaves exports needed at runtime in the runtime context.

If this is a feature request, what is motivation or use case for changing the behavior?

Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.

Details:

I have multiple components using react-redux, which import their actions. Like this:

import { connect } from 'react-redux';

import ComponentX from './ComponentX';
import { logout, do_auth as login, setAppToDashboard as goToDashboard } from '../../actions';

function mapStateToProps(state) {
  console.log('mapping ComponentX', { logout, login, goToDashboard }); // Defined here
  return { token: state.manager.token || false };
}
console.log('connecting ComponentX', { logout, login, goToDashboard }); // Undefined here
export default connect(mapStateToProps, { logout, login, goToDashboard })(ComponentX);

After moduleconcatenationplugin does its magic, my action imports are moved and concatenated (as expected) however they are moved so these actions are no longer available initially, so react-redux connect() gets an object of undefined values, rather than an object of functions.

Example of where they get moved to (compiled bundle):

/***/ }),
/* 17 */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
...
// CONCATENATED MODULE: ./src/actions/index.js
...
/* harmony export (immutable) */ __webpack_exports__["g"] = logout;
...
/* harmony export (immutable) */ __webpack_exports__["c"] = login;
/* harmony export (immutable) */ __webpack_exports__["k"] = setAppToDashboard;
...

This is happening in webpack 3.10.0, however I have reverted all the way back through releases to 3.6.0 and it still is exhibited. We've made it many months without this issue being exposed, and the only thing I can trace it to on our end is when our bundle gets large enough.

I will try to continue to narrow this down on my end, hopefully I will be able to create a small reproducible repo but currently that is unlikely. I am very happy to provide any other information or details, but keep in mind this is in a private proprietary application.

[pbarbiero]

Interesting, I have traced it to this change we made (reverting it resolves the issue):

No issues

new webpack.DefinePlugin({
  'process.env': {
    CUSTOM: JSON.stringify(customVar),
    NODE_ENV: JSON.stringify('production'),
    MODE: JSON.stringify(process.env.SYSTEM_ENV || 'production'),
    VERSION: JSON.stringify(pkg.version),
  },
})

Has issues:

new webpack.DefinePlugin({
  'process.env.CUSTOM': JSON.stringify(customVar),
  'process.env.NODE_ENV': JSON.stringify('production'),
  'process.env.MODE': JSON.stringify(process.env.SYSTEM_ENV || 'production'),
  'process.env.VERSION': JSON.stringify(pkg.version),
})

I was under the impression that the second way (the one with issues) was the safer, more correct way of doing things... We will revert that, but maybe its something that needs to be looked into?

[sokra]

Thanks for your report. It would be great if you reduce your issue to a small reproducible example. Best put this example into a github repository together with instructions how to get to the problem.

[pbarbiero]

I have tried to create a small reproducible application, but it seems its a complicated issue that occurs when a bunch of factors are met, which I haven't discovered yet. All I managed to prove was that most of what I disclosed here is inaccurate other than the end result.

I will see if I can pull apart our application to something I can disclose publicly, but its unlikely. I will close this issue ultimately if I cant figure out how to show you a reproduction.

[Sinewyk]

@pbarbiero Can you tell me your nodejs version ?

I'm hitting a similar bug where ModuleConcatenationPlugin is doing some stuff and breaking my resulting bundle, if I remove it everything works ... BUT, and here's the weird thing:

on node 8.8.1 it works, on node 6.10.3 it doesn't.

Exactly the same project, same branch, same commit, same dependencies (except built binaries). I suspect that ordering gets messed up by something internal to javascript. Just likes when you do Object.keys(obj) you get one order, you change node version, the ordering won't be the same, because the javascript vm ain't the same. That's just my theory.

@sokra any idea if some internals in webpack that rely on ordering (module concatenating for example 😏 ), actually use javascript functions to compute orders ... that are themselves not deterministic ?

Ex: Object.keys

[Sinewyk]

Ok wtf, I took the two files, I beautified them. Then ran them through a diff tool.

Node 8 on the left, node 6 on the right:

Both declare the export variable:

But the bundle resulting from node 6 is clearly broken compared to node 8:

The default assignment is just missing in the bundle built in node 6 😭

[pbarbiero]

Sure!

Interestingly, I am hitting this locally with 6.10.2, but also when done in our production server which runs 6.11.3 currently (which reminds me to update that)

Let me give it a shot with node 8 and see what happens.

[pbarbiero]

I still get the same problem running node 8.9.3, so not the same exact issue as you unfortunately.

[Sinewyk]

Well, I'll open another issue then. But still, it's confirmed for me. Switched on the "good" machine from node 8 to 6. Exact same error, ran it through beautifier + diff tool, and exact same line, the default export one, is missing now. I'll probably try to take a look at the source code to try to understand, but ...

[sokra]

That's really interesting...

Could you try if there is still a difference if you disable uglifyjs?

[pbarbiero]

For me there is no difference with or without uglifyjs.

I wonder if comparing the working bundle vs the non-working bundle (with the only difference being the defineplugin) would possibly help narrow it down and allow me to eliminate any proprietary data?

[Sinewyk]

@sokra Hu, 2 real differences, not uglified ( node 8 working on the left, node 6 broken on the right):

This is the t.a = vd that is missing after uglification on node 6 side

And it's also not writing __webpack_exports__["default"] = in the middle of the assignment farther in the file, but it's not that important (I mean, these 2 errors are probably linked).

So ... between node 6 and node 8, __webpack_exports__["default"] = gets skipped 😅

[Sinewyk]

I went and put a debug line here https://github.com/webpack/webpack/blob/master/lib/optimize/ConcatenatedModule.js#L741

console.log(dep.originModule.rawRequest, this.rootModule.rawRequest, dep.originModule === this.rootModule);

On node 8, the interesting line is
./coreApp ./coreApp true
on node 6 it's
./coreApp ./coreApp false

This causes the __webpack_exports__["default"] = to get left out, because we don't enter the if () block adding it.

Not sure yet why

[pbarbiero]

I tried comparing my working bundle vs the non-working bundle, and interestingly with the only change being how I use DefinePlugin, my two bundles are MASSIVELY different. Like, incredibly significantly different across the board.

Heres a small example related to my initial report:

Working:

/* harmony default export */ var Component = (connect_connect(Component_mapStateToProps, { logout: actions_logout, login: do_auth, goToDashboard: setAppDashboard })(Component_Component));

Broken:

/* harmony default export */ var Component = (Object(react_redux_es["b" /* connect */])(Component_mapStateToProps, { logout: actions["g" /* logout */], login: actions["c" /* do_auth */], goToDashboard: actions["k" /* setAppDashboard */] })(Component_Component));

In addition, the order of concatenated modules are wildly different, not to mention lines and characters (these are both with uglify disabled)
Working: 101168 lines, 3406874 characters
Broken: 101450 lines, 3425874 characters

[Sinewyk]

I'm going to spend my day trying to figure out the difference in behavior between node 6 & 8 I think.

Started with adding a debug line

console.log(
  dep.originModule.rawRequest,
  this.rootModule.rawRequest,
  dep.originModule === this.rootModule,
  dep.originModule.rawRequest === this.rootModule.rawRequest
);

after that if block

webpack/lib/optimize/ConcatenatedModule.js

Line 736 in 30329fb

if(dep.originModule === this.rootModule) {

and saving the output of my build

As I thought, again for my "coreApp" this is what I get (broken 6 left, working 8 right)

Everything seems to be resolved in the same order, but the result of the path resolving are not the same (node path algorithm changed ?) or maybe it's just random, between builds of exact same setup I also see some differences in path resolution.

I've got 16 of 896 debug lines being different, sometimes the originModule, sometimes the rootModule.

But only 2 lines truly broken with that boolean check dep.originModule === this.rootModule being false when it should be true.

First one my "coreApp" stuff, and another one.

More interesting, I started looking into the reference equality check === ... so I kept a global count of all the references. in a global array to want to do some check at the end ... like are all of them equal for a same rawRequest: the bug disappears.

Trying to look into the why the reference equality fails ... make it disappear (when keeping references to objects), garbage collection strikes again, wtf ? WeakSet or WeakMap in the codebase ?

[Sinewyk]

Come on, I literally need to do just

if (dep.originModule.rawRequest === './coreApp') {
  global.myArrayDep.push(dep.originModule);
}

to make the bug disappear 🤣

@sokra Are you keeping hard references of all modules or are you maybe losing/recreating them along the way to spare hard memory limits ? WeakMap, WeakSet usage ? Anything comes to mind as to why keeping a hard global reference to a module would make that equality reference check just work ?

[sokra]

Afaik Javascript is specced in a way that GC is not visible to the user. Keeping a global reference to an object should not change the behavior (expect for more memory used).

cc @bmeurer

[sokra]

btw. rawRequest depends on timing, but it's not used anyway. (I should remove it...)

---

I could imaging this bug is caused by the unstable behavior in sort()...

[Sinewyk]

I added

this.createdat = Date.now();

in the constructor of DependenciesBlock to check timestamps when the reference equality check fails: timestamps are still equals.

Something is making a reference equality check fails, even though it's the same objects 😆

Node 6 is broken :/ and node 8 somehow fixed the issue, even though yesterday I didn't see anything in the v8 bug tracker or nodejs changelog that could reflect that. But I hope I'm mistaken.

I'm gonna do a crude bisect between node versions to try to narrow down the issue 😕

edit:

version	status
6.10.3	broken
6.12.2	broken
7.0.0	good
7.10.1	good
8.9.3	good

[Sinewyk]

See previous comment edit, but that means something (was fixed in || was broken before) 7.0.0, and most notably https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V7.md#notable-changes-14

V8 has been updated to 5.4.500.36

Is it worth it looking farther than that or just mark node < 7 unsupported ?

edit: I didn't see anything noteworthy in the changelog, but let's keep in mind that apparently the changelog itself was broken between 4.10.253 and 5.2.49 ...

2016-04-18: Version 5.2.49

        [api] Bring back finalizers on global handles.

        Performance and stability improvements on all platforms.

2016-02-17: Sentinel

        The ChangeLog file is no longer maintained on master. This
        sentinel should stay on top of this list.

2016-02-17: Version 4.10.253

        Performance and stability improvements on all platforms.

[sokra]

It would be great if you look further. We can't mark node 6 as unsupported.

[Sinewyk]

Well, I've cloned the v8 mirror from github, I've looked through all commits between 5.1.281.109 and 5.4.500.36 looking for (grep) "Object" or "equal" or "reference" and I didn't find anything useful.

For my use case I'll just upgrade to node 8 LTS asap (and while waiting just disable the ModuleConcatenationPlugin) and I'll be good to go.

And by unsupported I meant maybe just a note for the ModuleConcatenationPlugin. It seems to work ok for small projects and seem to break around 800 modules or something like that.

Not sure what more I can do when upgrading nodejs fixes the issue.

[pbarbiero]

Closing this as it hasnt been an issue for me lately and there have since been a lot of changes to webpack.