AMD modules unnecessarily uses exports

[aapoalas]

• Rollup Version: 1.15.1
• Operating System (or Browser): Ubuntu 14.04 / Chrome 74.0.3729.157
• Node Version: 8.10.1 / Repl

How Do We Reproduce?

Export non-circular, named dependencies from a file and set the output format to AMD.
Repl link

Expected Behavior

Since the file does not contain any circular logic, there is no reason for the AMD module to import the special "exports" dependency. A simple return statement would do.

Actual Behavior

The "exports" dependency is set without a fail.

[lukastaegert]

Just curious as I am not an experienced AMD user: What is the relation between "circular logic" and the "exports" dependency? In which situations would returning an object fail and you need to use "exports"?

[aapoalas]

Hello! I'll try to explain to the best of my knowledge. The reference here is from RequireJS's API documentation: https://requirejs.org/docs/api.html#circular

I'll try to use this nonsensical Repl as an example: link

So to put it simply, if for some reason we have two modules that depend on each other (or more files that form a dependency circle, eg. a -> b -> c -> a), then when the loading reaches the circular point there is no way to resolve the latest load.

In our example (thinking as AMDs), we have loaded "a.js" (think of this as dynamic import, so a Promise) but that file lists a dependency "b.js". So before we can call the actual factory function of "a.js" we need to load "b.js". So we do a dynamic import for "b.js" and get an AMD file that lists "a.js" right back at us as a dependency. We've already loaded "a.js" and we probably have a reference to the Promise that it returned, but we're actively trying to resolve that Promise and the best we can do here is to throw or leave the loading Promise forever unresolved.

However, if "a.js" had a dependency "exports", then RequireJS would create a new object for "a.js" and inject that as dependency to the module factory while also making note that this module is now equal to that particular object. Now when we load "b.js" and see that "b.js" requires "a.js" again, we can simply resolve that dependency by giving the module factory the object that earlier was attributed to "a.js". Now, admittedly when we call "b.js" with this object, it doesn't contain anything yet. We just need to hope that no exports are, as of yet, used when the factory is first called. The return value (or exports object if 'exports' was listed as a dependency again) of "b.js" will then be marked down and passed to "a.js"'s module factory. Finally "a.js"'s module factory gets to run and populate the object that was created as the "exports" object earlier.

[lukastaegert]

Thanks, that makes sense. So I guess the use of the exports dependency was added as a precaution to be able to map circular dependencies to AMD modules. The problem is that it is hard to determine conclusively if there are circular dependencies. Even if there are none in the code-base itself, as soon as there is an external dependency there is nothing preventing the external dependency from importing our module and creating a cycle.
So this would be an optimization for "no cycles and no external dependencies".

[aapoalas]

Mmm, my personal opinion is that Rollup shouldn't worry about external dependencies causing dependency cycles. As long as Rollup's chunking algorithm doesn't create cycles on its own (or can somehow marks these cycles to use exports in AMD format cases), then everything beyond that should be on the user.

At the very least, the current case makes very little sense where default exports turn into plain returns and named exports instead use the 'exports' object. Both export types are equally liable to causing non-resolvable cycles with AMD modules.

[lukastaegert]

My personal opinion is that Rollup should preserve the behaviour of ES modules as well as possible, and circular dependency support is a feature, even though it should be handled with care. It is rather trivial to create cycles with code-splitting:

https://rollupjs.org/repl/?version=1.16.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QnglN0QlMjBmcm9tJTIwJy4lMkZvdGhlci5qcyclM0IlNUNuJTVDbmV4cG9ydCUyMGZ1bmN0aW9uJTIweSgpJTIwJTdCJTVDbiU1Q3Rjb25zb2xlLmxvZyh4JTJDJTIwJ3knKSUzQiU1Q24lN0QlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyb3RoZXIuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCeSU3RCUyMGZyb20lMjAnLiUyRm1haW4uanMnJTNCJTVDbiU1Q25leHBvcnQlMjBmdW5jdGlvbiUyMHgoKSUyMCU3QiU1Q24lNUN0Y29uc29sZS5sb2coJ3gnJTJDJTIweSklM0IlNUNuJTdEJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmFtZCUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

To me it sounds more like a feature request for a flag that will reduce compatibility.

[aapoalas]

Mm, yeah. I guess you're right. Only thing that can be then counted as a bug is the difference in AMD module format between named exports and default exports. But that bug is (at least to me) very beneficial for as long as there is no flag to reduce compatibility :)

https://rollupjs.org/repl/?version=1.16.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMHglMjBmcm9tJTIwJy4lMkZvdGhlci5qcyclM0IlNUNuJTVDbmV4cG9ydCUyMGRlZmF1bHQlMjBmdW5jdGlvbiUyMHkoKSUyMCU3QiU1Q24lNUN0Y29uc29sZS5sb2coeCUyQyUyMCd5JyklM0IlNUNuJTdEJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMm90aGVyLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMHklMjBmcm9tJTIwJy4lMkZtYWluLmpzJyUzQiU1Q24lNUNuZXhwb3J0JTIwZGVmYXVsdCUyMGZ1bmN0aW9uJTIweCgpJTIwJTdCJTVDbiU1Q3Rjb25zb2xlLmxvZygneCclMkMlMjB5KSUzQiU1Q24lN0QlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyYW1kJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

[mgol]

We've just switched the jQuery source from AMD to ES modules and modified our build process to rely on Rollup. We'd still want to generate a separate amd directory with transpiled AMD sources so that it's still possible to consume individual AMD modules as it used to be possible until recently. Unfortunately, the way Rollup generates those AMD modules is incompatible with the original interface that we had that just returned a single thing from each module. While the entry file gets converted in the equivalent way (at the end it does return core.default), we want to support our users using our individual AMD modules directly and now that'd become quite inconvenient and - from what I've heard - not idiomatic to how AMD is typically used.

One example is the var/document.js file that used to be:

define( function() {
	"use strict";

	return window.document;
} );

and would now be:

define(['exports'], function (exports) { 'use strict';

	var document = window.document;

	exports.default = document;

});

With the new form, we'd have to require jQuery consumers to read the default property from every module they import.

The original semantics are important to jQuery so if Rollup doesn't support this use case we'll either have to find another tool for this part of the build or post-process Rollup output which can be brittle.

[aapoalas]

Hum. At least previously export default foo would turn into a simple return. Like here.

But I guess this may have changed in newer versions that are not reflected on the website?

[lukastaegert]

No, the website is always using the latest version by default. An AMD module returning only a default export should always use a return statement. It should only switch to using exports when named exports are involved.

[mgol]

@aapoalas That's the main module, though; I'm transforming individual modules.

You can try it yourself by cloning jQuery, running:

rollup --preserveModules --dir amd --format amd --input src/jquery.js

and seeing what's generated in the amd directory. The module I mentioned will be at amd/var/document.js.

jQuery source doesn't have import cycles which I just verified using eslint-plugin-import's no-cycle rule. All the modules only use default exports so no named exports are involved.

[lukastaegert]

preserveModules may not be what you want as it can also remove exports due to tree-shaking, but besides that, I agree, preserveModules should probably maintain entry point semantics for all files, and maybe that is the important bug that should be addressed for you—which is slightly different from the original issue, though, as that one is about named exports in entry modules. It might be worth opening a separate ticket for that, referencing the connection with preserveModules.

What WILL work (and might be better suited for you?) is generating an input object e.g. via script that promotes all valid sub-entry points to proper entry points. Another advantage here would also be that modules containing internal functionality could be excluded from this and be merged with other modules for a more optimized output.

In a way, I wrote an article describing that pattern: https://levelup.gitconnected.com/code-splitting-for-libraries-bundling-for-npm-with-rollup-1-0-2522c7437697

[mgol]

@lukastaegert I've reported the issue with preserveModules at #3258.

I've also tried your suggestion of not using preserveModules & instead generating an input array containing all the files but the resulting modules have broken paths and they import everything directly instead of keeping original imports.

Compare the source at src/jquery.js with what it compiles to at amd/jquery.js. For example, the first entry in the array passed to define is './arr' when - first of all - it shouldn't be mentioned in that file at all as it comes from src/core.js, and - secondly - the path should be './var/arr'.

Code that generated these files is at build/tasks/amd.js.

Let me know if you want to move this part of the discussion elsewhere as it's getting off topic.

[dasa]

instead generating an input array

I think you should be using an input object instead.

[lukastaegert]

Exactly. The paths are not broken by Rollup, they are broken because you move them around after generating the output and do not preserve their relative positions. Rollup's output is meant to be kept as it is concerning the relations of the modules. If you want generated chunks to be in nested paths, an input object can help:

input: {
  'var/arr' : 'src/var/arr.js',
  // ... other entry points
}

With the default configuration, the output object will then look like

{
  'var/arr.js': { code: ..., ...},
  ...
}

And imports from to and from other chunks will be resolved correctly. Nevertheless I see that you are actually making EVERYTHING an entry point, in which case there would be no advantage to this technique over preserveModules (once we fix the AMD output as outlined). Is it really useful to directly import EVERY file? If this is not the case, then maybe omitting a list of files and directories that do not need to be directly imported would result in a much more optimized output by letting Rollup merge these into other modules efficiently. AMD is a slightly verbose format where any such optimization would immediately result in a smaller and more efficient output.

[mgol]

Is it really useful to directly import EVERY file?

I wanted to first make sure our existing setup is replicable, especially that we'll also expose our new ES modules and if we want to bundle AMD in what we publish into a few entry points we'll have to do that with ESM as well - and we don't bundle ESM at all right now (except for producing the single final file).

I'll try your suggestions, thanks.

[khangsfdc]

No, the website is always using the latest version by default. An AMD module returning only a default export should always use a return statement. It should only switch to using exports when named exports are involved.

If exports is required to handle circular logic in AMD format and Rollup doesn't use exports for default export (outside of configuring output.exports), would circular logic be broken in this scenario?

ESM module handles circular deps correctly:

import _testCircularModule from 'recursive_dependency_bare';
const obj = {
  factory() {
    return _testCircularModule;
  }
}
export default obj;

Rollup converted AMD format which would not handle circular dep:

define(['recursive_amd_bare'], function (_testCircularModule) { 'use strict';

  _testCircularModule = _testCircularModule && _testCircularModule.hasOwnProperty('default') ? _testCircularModule['default'] : _testCircularModule;

  const obj = {
    factory() {
      return _testCircularModule;
    }
  };

  return obj;

});

See systemjs/systemjs#2036

If i'm understanding this correctly, the only way to get the equivalent ESM functionality would be to set output.exports to named (to force exports object).