Simplify MODULARIZE code

[sbc100]

We had a report the use of exports["Foo"] rather then exports.Foo was causing problems.

Can we just remove this line completely and rely on module.exports to take care of the CommonJS case?

[sbc100]

Adding @dcodeIO who wrote this code back in #6006

[RReverser]

We had a report the use of exports["Foo"] rather then exports.Foo was causing problems.

Huh I wonder why. Some static analyzer not understanding the former?

Can we just remove this line completely and rely on module.exports to take care of the CommonJS case?

We kinda can. Kinda because technically module.exports is a Node.js thingy and not part of the CommonJS spec - that's why exports.something = ... was often used if module doesn't exist, but I believe it doesn't matter nowadays as pretty much all the tooling supports module.exports, and Emscripten targets only Node.js among CommonJS runtimes anyway.

[kripken]

Don't we need this for closure? That is, won't closure change exports.Foo into a.b or such? Unless exports is specially recognized by closure compiler.

[sbc100]

Don't we need this for closure? That is, won't closure change exports.Foo into a.b or such? Unless exports is specially recognized by closure compiler.

But here I'm just removing the line completely, not removing the quotes.

The module.exports = Foo line two lines up should be enough IIUC.

[kripken]

I see. Could be, I don't know enough about the details of JS module systems.

[dcodeIO]

The purpose of the quotes indeed is that closure does not mangle the property name. Only relevant when using advanced optimizations plus there are no externs describing the property otherwise.

The code in question did the following before the change:

• In a NodeJS environment, both exports and module are present in a CommonJS module context. There, one can do module.exports = ... to replace the singleton value of exports, since just assigning another value to exports would not become visible to the outside. Might also be that exports is const nowadays.
• In an AMD environment, use define to announce the module.
• In any other CommonJS environment (or where there otherwise is a declared exports variable) where there is no module to utilize the trick above, add a property to the exports variable since, as mentioned above, reassigning the module-local exports variable does not become visible to the outside - hence a property on top.

The last case is arguably the least important, but I can't tell how little. Iirc this code originated as a variation of Universal Module Defintion. Uses I know of would not be broken when removing the last case.

[sbc100]

What kind of CommonJS environments are there that don't have module (i.e. are not node?) Do we support running in such environments?

Given that the node also support the third case, could we drop the first case (module.exports = ) and always use exports.Foo ? Is there any benefit to having two different code paths here?

[sbc100]

Looking at the closure docs it seems like it actually recommends doing exports = {Foo}. https://github.com/google/closure-library/wiki/goog.module:-an-ES6-module-like-alternative-to-goog.provide#named-exports

Do you know why they seem unconcerned by the "variable does not become visible to the outside" issue you mention?
On what platforms is that that problem?

[dcodeIO]

Iiirc the const exports = module.exports aspect used by Node isn't part of the CommonJS "spec". I found that the wiki for example states that 'modules must use the "exports" object as the only means of exporting', though, somehow, assigning to module.exports, whether intentionally provided by Node or not, turned out to be useful to people where there is an existing value that should become the module's exports. Allows to export just a single function for example (I think Emscripten did that at some point?), or assemble the exports object otherwise, which later was superseded by a default export in ESM. Interestingly, however, the CommonJS wiki also states 'In a module, there must be a free variable "module", that is an Object' (see), even though it doesn't mention an .exports property on it. Somewhat a case of whatever we do, we are doing it slightly wrong anyway. Other than that, the last case seems to not do much in practice anyhow, so I would not be opposed to remove it - though I wonder what kind of errors occurred with it, as it might be that the removed branch actually executes in affected setups, only then leading to issues? In that case, removing it would not really fix things, but simply make the module export nothing instead of erroring.

[dcodeIO]

As a bit of speculation: One possible error here is that if neither the first (CommonJS) nor the second branch (AMD) is taken, and exports = null, which is still typeof "object" in JS, then assigning to exports.something will produce an error at runtime. A situation this could happen in is if a non-ES6 Emscripten artifact is included in a <script ...> tag, and earlier var exports = null is set by other code globally. The question then becomes: What should ideally happen in this case? Say, if the third branch was removed, the module would load, but would it function?

A workaround specific to this case would be

-else if (typeof exports === 'object')
+else if (typeof exports === 'object' && exports)
   exports["%(EXPORT_NAME)s"] = %(EXPORT_NAME)s;

but it's probably also not really fixing anything, since if exports = null that's identical to just removing that case.

[sbc100]

Would we not wrap all of this code in if (!ENVIRONMENT_IS_BROWSER) since IIRC browsers don't support these modules anyway?

Also, can you explain more what you we can't just do exports = {Foo}?

[dcodeIO]

I believe the reason exports = {Foo} doesn't work is because what's happening under the hood is something like

function initModule(module, exports) {
  // module code is inserted here
  return module.exports;
}
initModule(module, module.exports);

where an assignment exports = {Foo} in module code only mutates exports locally, but does not affect module.exports.

I don't know enough about the ENVIRONMENT_IS_* constants unfortunately. A quick search for ENVIRONMENT_IS_BROWSER yielded no results in code.

[sbc100]

Sorry I meant ENVIRONMENT_IS_WEB. I guess we can't do that though because we actually do want this code to runnable on the web and folks use module loaders/bundlers to run modules on the web ?

[sbc100]

I believe the reason exports = {Foo} doesn't work is because what's happening under the hood is something like

function initModule(module, exports) {
  // module code is inserted here
  return module.exports;
}
initModule(module, module.exports);

What platforms/toolchain do you think do this? I'd like to test it out (and add some tests to emscripten).

[dcodeIO]

Sorry I meant ENVIRONMENT_IS_WEB. I guess we can't do that though because we actually do want this code to runnable on the web and folks use module loaders/bundlers to run modules on the web ?

Hmm, yeah, removing everything would produce non-functional artifacts if such a wrapper is present. Though, I guess a bundler would adhere to what Node does (for Node modules), so removing the third branch, or fixing it as of above if that's indeed the error seen, would not break the wrappers.

What platforms/toolchain do you think do this? I'd like to test it out (and add some tests to emscripten).

In Node, which I guess is somewhat the source of truth, the relevant code seems to be somewhere around here:

const wrapper = [
  '(function (exports, require, module, __filename, __dirname) { ',
  '\n});',
];

where what is returned ultimately is the module's exports property, similar to here.

I suggest that, if the reported error is something like TypeError: Cannot set properties of null, to adjust the check in the last branch to typeof exports === 'object' && exports as this doesn't break existing (and working) uses, or otherwise to remove the last branch.

[sbc100]

The oldest version of node that we support is v10.19.0 and I confirmed that both module and exports exist as empty objects on the global scope on that version of node.. so the first branch of this code will always be taken. So I think we can safely go ahead and land this change as-is.

[sbc100]

@kripken @RReverser can one of you lgtm?