Lazy circular dependencies no longer work

[patrick-steele-idem]

It looks like the changes that were part of #92 broke lazy circular dependencies. After investigating I found this to be the cause for #104

Lazy circular dependencies work with Node.js and other CommonJS module loaders/bundlers. I think it is important to maintain parity with the Node.js CommonJS module loader since this is breaking at least one library.

Given the following contrived setup where ./a.js lazily requires ./b.js and ./b.js requires ./a.js:

_./a.js:_

setTimeout(function() {
    var b = require('./b'); // ** Lazily require './b.js' inside `setTimeout()` callback **
    console.log(b);
}, 10);

module.exports = 'SUCCESS!';

_./b.js:_

module.exports = require('./a');

Running the following code under Node.js using node ./a.js will result in the following output:

SUCCESS!

However, running the code bundled up by rollup will output undefined.

Problem

Rollup produces the following code that eagerly loads each module:

(function() {
    'use strict';

    function createCommonjsModule(fn, module) {
        return module = {
            exports: {}
        }, fn(module, module.exports), module.exports;
    }

    var b = createCommonjsModule(function(module) {
        module.exports = a; // ** a is undefined at this point **
    });

    var a = createCommonjsModule(function(module) {
        setTimeout(function() {
            var b$$1 = b;
            console.log(b$$1);
        }, 10);

        module.exports = 'SUCCESS!';
    });

}());

Proposed solution

I propose that this plugin be updated to produce output code similar to the following:

(function() {
    'use strict';
    function defineCommonjsModule(fn) {
        var module = {
            exports: {}
        };

        var loaded;

        return function require() {
            if (!loaded) {
                loaded = true;
                fn(module, module.exports);
            }
            return module.exports;
        };
    }

    var $$require_b = defineCommonjsModule(function(module, exports) {
        module.exports = $$require_a();
    });

    var $$require_a = defineCommonjsModule(function(module, exports) {
        setTimeout(function() {
            var b = $$require_b();
            console.log(b);
        }, 10);

        module.exports = 'SUCCESS!';
    });

    $$require_a(); // ** Load the entry module **
}());

As a side benefit, the proposed code will be closer to the user's code while still being very concise and efficient.

Thoughts?

/cc @Rich-Harris @lohfu

[Rich-Harris]

I've been staring at this for a while and struggling to understand what actually changed between 3 and 4. The example above doesn't work in either 3 or 4, so whatever's going on with Marko is probably something different. If anyone is able to pin it down that would be very helpful!

@patrick-steele-idem I really liked the look of your proposed solution. Unfortunately I just can't get it to work. The problem is that in most Rollup bundles, a CommonJS file will be imported by an ES module, which means that the default export from the CommonJS module can't be a function that returns module.exports, it has to be module.exports.

We can distinguish between CommonJS importers and ES importers (CJS modules import the __moduleExports name via an interop proxy module, whereas ES modules just import the default export), but that doesn't help us because the module has to call the lazy function immediately in order to create the default export.

I might well be missing something obvious, and I'm certainly open to ideas! I just think this might be a fundamental difference between declarative and imperative module systems.

None of which is to say we shouldn't be able to fix the Marko regression, whatever it is...

[patrick-steele-idem]

I can't say what the regression was or even if there was a regression because I only investigated the problem with v4 and found it to be due to the eager loading of modules.

I haven't had a chance to take a closer look at CJS/ES modules interop, but would using a getter (IE9+) help? Something like the following:

(function() {
    'use strict';
    function defineCommonjsModule(fn) {
        var module = {
            exports: {}
        };

        var loaded;

        return {
            get default() {
                if (!loaded) {
                    loaded = true;
                    fn(module, module.exports);
                }
                return module.exports;
            }
        };
    }

    var $$module_b = defineCommonjsModule(function(module, exports) {
        module.exports = $$module_a.default;
    });

    var $$module_a = defineCommonjsModule(function(module, exports) {
        setTimeout(function() {
            var b = $$module_b.default;
            console.log(b);
        }, 10);

        module.exports = 'SUCCESS!';
    });

    $$module_a.default; // ** Load the entry module **
}());

[Rich-Harris]

I haven't had a chance to take a closer look at CJS/ES modules interop, but would using a getter (IE9+) help?

Unfortunately not – the only way for that to work would be if all references to default imports in ES modules (whether or not from CJS modules, because the importing module doesn't know) were rewritten (foo -> foo.default), and if you were going to do that, a) you may as well just rewrite them as call expressions, and b) it would slow down all bundles for the sake of this edge case. Will keep thinking...

[patrick-steele-idem]

I could be wrong, but I feel like making imports a call expression would not introduce any measurable slow down. The code would not be "hot" code since importing/requiring a module would typically only be done during initialization. I feel like correctness is more important here...maybe it's worth fixing the problem now and trying to optimize more later?

[Rich-Harris]

It would create slowdown, yes – if you read the cost of small modules you'll get some idea of the potential downside. Bear in mind that this isn't just something that would have to happen for CommonJS modules, it's something that would have to happen for all modules in all bundles, whether or not they use rollup-plugin-commonjs, and the implementation would be fighting against the semantics of ES modules. It would also make it very hard for Rollup to treeshake bundles that had been generated with Rollup!

If this were a very widespread issue (or impossible to solve another way) it might be a different matter, but as it is I'm fairly sure it would create an unacceptable overhead in both bytes and performance. A good workaround would be to a) bundle up the CommonJS modules in question beforehand (using Browserify or Webpack) and importing that bundle (really, the libraries in question should already be providing those distributables), or b) generate a CommonJS bundle that excludes the offending modules and hand that off to Browserify/Webpack for the final step.

Better still, lobby the library authors to switch over to JavaScript modules!

[lukeapage]

just got this problem with winston logging framework, also uses circular dependencies and took a long time to track down..

[sadovnychyi]

Seems to be a case with searchkit as well. Any updates on this?

[bitjson]

Same issue with this module: indutny/hash.js#6 (and the author specifically does not want to remove circular dependencies).

A good workaround would be to a) bundle up the CommonJS modules in question beforehand (using Browserify or Webpack) and importing that bundle (really, the libraries in question should already be providing those distributables)

@Rich-Harris – could this be done as an option in rollup-plugin-commonjs? Or maybe this is a good job for another plugin? If Rollup users could indicate a list of modules to "browserify first", it would make using Rollup a lot less stop-and-go.

My current workaround is to install browserify as a project dependency, browserify the offending module, and import it from a "temp" folder from my code. But, that really breaks the cleanliness and flow of using Rollup.

[dfrencham]

I've been bitten by this. I spent a good few hours trying different approaches to package Winston logger, with no luck.

[shellscape]

Hey folks (this is a canned reply, but we mean it!). Thanks to everyone who participated in this issue. We're getting ready to move this plugin to a new home at https://github.com/rollup/plugins, and we have to do some spring cleaning of the issues to make that happen. We're going to close this one, but it doesn't mean that it's not still valid. We've got some time yet before the move while we resolve pending Pull Requests, so if this issue is still relevant, please @ me and I'll make sure it gets transferred to the new repo. 🍺