NamedModulePlugins breaks global lookups, such as jquery

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

What is the current behavior?
Adding NamedModulesPlugin to a project that uses certain global lookups results in what are effectively reference errors.

If the current behavior is a bug, please provide the steps to reproduce.
Add NamedModulePlugins to https://github.com/shakacode/bootstrap-loader/blob/master/examples/basic/webpack.dev.config.js (which uses jquery & bootstrap. Resulting Error).

https://github.com/eggheadio/egghead-ui/blob/master/src/components/Icon/index.js#L65 also breaks with keys being declared not a function.

What is the expected behavior?
That global lookups work.

If this is a feature request, what is motivation or use case for changing the behavior?
Compatibility with other libraries.

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

Author: Fantashit

2 thoughts on “NamedModulePlugins breaks global lookups, such as jquery

  1. I tracked this down and I believe it’s a bug in NamedModulesPlugin, not expose-loader. When I looked in the bundle file causing errors, I saw that there were two modules with the same ID (the path to jquery on the filesystem). NamedModulesPlugin works by calling module.libIdent on each module (code) and using that as the ID, but it looks like in some cases (e.g. modules created by expose-loader), two different modules return the same libIdent return value, resulting in an ID collision. I hacked the code to keep track of all generated IDs and skip any cases where the same ID has been generated before, and it fixed the issue.

    I can try coming up with a PR, although I’m fairly new to webpack, especially the internals. But it seems like it should be an easy fix. @sokra do you want a new issue filed, or do you think it makes sense to re-open this issue.

  2. I have zero experience with Webpack’s source code (except for reading it to figure things out when docs fall short). So I probably don’t know what I am talking about. Take it with a grain of salt:

    So a patch like what @alangpierce described would make NamedModulesPlugin look like:

    /*
        MIT License http://www.opensource.org/licenses/mit-license.php
        Author Tobias Koppers @sokra
    */
    "use strict";
    
    class NamedModulesPlugin {
        constructor(options) {
            this.options = options || {};
        }
    
        apply(compiler) {
            compiler.plugin("compilation", (compilation) => {
                compilation.plugin("before-module-ids", (modules) => {
                    let usedIds = [];
                    modules.forEach((module) => {
                        let proposedId = null;
                        if (module.id === null && module.libIdent) {
                            proposedId = module.libIdent({
                                context: this.options.context || compiler.options.context
                            });
                            if (!usedIds.includes(proposedId)) {
                                module.id = proposedId;
                                usedIds.push(proposedId);
                            }
                        }
                    });
                });
            });
        }
    }
    
    module.exports = NamedModulesPlugin;

    This is, in fact, easy and fixes the expose-loader issue described at webpack-contrib/expose-loader#55 . But I sense that skipping setting module ids might break consistent hashing for long-term caching. Since we ended up skipping setting module ids for certain modules, I assume the ids for such modules became non-deterministic.

    Let’s wait for @sokra to weigh in.

Comments are closed.