Changes the checks to prevent processing files multiple times

[arthurjamain]

Hey there,

I has opened an issue about incompatibility with another plugin. I'm not sure how I diagnosed stuff that day, but boy was I confused. To my defence, our build system is a bit of a behemoth and it's hard to pinpoint exact interactions in a vacuum.

Anyways, I've re-dived into the subject and found that my initial assessments were absolutely wrong. I've worked backwards from the symptoms towards a solution that works for me, but honestly I haven't properly understood how all of the plugin works, so what I'm proposing here might be dumb, even though it seems to do the deed for me.

The symptoms were :

• The hashes are indeed generated and are present on the compilation object for all the entry points
• When loading async chunks, they are inserted with the magicMarkers instead of the actual hash that's been computed for them. Basically, almost all of the placeholders are still there, except for a few values.

My config, roughly, is as follows :

• scss/postcss/extracttext, ulgify, assets manifest, etc. mostly classics
• three levels of commonchunk plugin which I suspect is the true issue here. For various cache/performance related reasons, we have three imbricated levels of commonchunks for different parts of our app. I think this either means that some events are triggered several times instead of once, or that it tricks the plugin in thinking that it processed the common chunk n instead of n2. While logging the process, I noticed that the "root" common chunk file was processed three times.

This PR replaces the original check with one further down the line. It checks whether parentChunk.id + childChunk.id were updated instead of just parentChunk.id ; that was there is not stone left unturned.

Of course this also means that the code is probably less performant. To me this hasn't been an issue, but I guess it could be possible to have an infinite loop or something along those lines, given the form of the existing code. You know that was better than I do anyway.

Hope this helps.

[jscheid]

Hi, thanks for this.

You've probably read this document which explains that we can't merge code without a test case.

I'll try to find time to reproduce the issue based on what you wrote above but I have a busy week ahead of me. If you want to help move things along, a test project that demonstrates the problem would be extremely useful.

[jscheid]

Also, this breaks the existing tests with Uncaught RangeError: Maximum call stack size exceeded... that's probably the infinite loop you were guessing at.

[arthurjamain]

Hey,

Yeah thanks, that makes sense. Honestly, I'm pretty sure this PR is not an actual solution, the check it removes seems important and I probably don't need it in my specific case. I'll add on top of it as soon as possible, but in the meantime I have to move forward on the subject I was implementing this plugin for.

I'll be back :) Cheers.

[jscheid]

Can you send your commonChunk config that shows how exactly you use it three times?

[arthurjamain]

Sure, here it is :

    new CommonsChunkPlugin({
        name: 'viewer',
        filename: commonsChunkfilename,
        chunks: [
            'editor',
            'generatorViewers',
            'standaloneViewer',
            'screenshotGeneratorViewer',
            'embed'
        ]
    }),

    new CommonsChunkPlugin({
        name: 'website-commons',
        filename: commonsChunkfilename,
        chunks: [
            'home',
            'explore',
            'profile',
            'settings',
            'model',
            'static',
            'categories',
            'tags',
            'upgrade',
            'newsfeed',
            'auth',
            'store',
            'search'
        ],
        minChunks: function(module, count) {
            if (
                _moduleContainsPath('sources/std/', module) ||
                _moduleContainsPath('/sources/utilities/', module)
            ) {
                return true;
            }
            return count >= websiteCommonsChunks.length;
        }
    }),

    new CommonsChunkPlugin({
        name: 'commons',
        filename: commonsChunkfilename,
        chunks: ['viewer', 'playlists', 'website-commons', 'onboarding'],
        minChunks: function(module, count) {
            return count >= commonsChunks.length && _moduleContainsPath('/node_modules/', module);
        }
    }),

Basically :

• website-commons is the commons for all of the "platform"-related stuff ; it's used everywhere our platform serves "traditional" content.
• viewer is the commons for all of our webgl code, which is served on some specific pages (no strict correlation with website-commons)
• commons is the commons from both of those + two other legacy entries that are not part of the aforementioned commons.

It's a bit convoluted but it seems efficient. If that sounds dumb I'd be glad to hear it though :)

[jscheid]

Oh, I'm not here to judge your config :-) I just want this plugin to work with any config that works without it. Thanks for sharing it.

Do I understand correctly that your issue doesn't have anything to do with webpack-manifest-plugin after all?

[arthurjamain]

Yes, I'm not sure how I originally came to this conclusion (probably too much random tries / local env bullshit), but I could definitely reproduce the exact same issue with or without the webpack-manifest-plugin.

[jscheid]

@arthurjamain I can reproduce with multiple CommonsChunkPlugins (with a test based off https://github.com/webpack/webpack/tree/master/examples/multiple-commons-chunks).

[jscheid]

@arthurjamain could you try with #59?

[arthurjamain]

Hey, thanks for your patch ! I've just tried a couple times with and without the patch in #59 . With the patch, webpack seems to get stuck (in what I assume to be an infinite loop, given that webpack uses 100% CPU) on the asset optimization phase (94%).

Reading your test case, the only significant difference I can see is that in my config, one of the commonchunk plugins actually uses the output of another commonchunk plugin as in input.

One of the common chunks is both a source and an output. If I understand correctly, this means that the plugin would have to backtrack in its processing to work properly. I'm starting to wonder if that type of config is compatible at all with this process. I'll try to debug with your patch a bit and see where it fails.

[jscheid]

Ah, that's disappointing.

A chunk being both source and output shouldn't be a problem. We even have a test case for handling mutually dependent chunks gracefully.

I've tried reproducing by tweaking my test case in different ways (making commons1 depend on commons2, vice versa, and both at the same time, and having a third chunk depend on both) but can't seem to get it to go into an infinite loop. I'm afraid I'll have to rely on you digging a bit deeper.

Perhaps you could start just by Ctrl+C and send me the stack trace?

[jscheid]

@arthurjamain whatever works for you, but couldn’t you edit directly in node_modules or use npm link?

[arthurjamain]

Whoops, sorry forgot this PR existed, will close.

As to why I'm doing it this way, unfortunately it's necessary given my dev environment ...

[jscheid]

@arthurjamain fix for this is released in v1.0.4.