Isomorphic code is no longer isomorphic

[SupernaviX]

• Rollup Plugin Name: commonjs
• Rollup Plugin Version: 11.1.0
• Rollup Version: 2.7.3
• Operating System (or Browser): Windows 10
• Node Version: 13.9.0

How Do We Reproduce?

Repro repo at https://github.com/SupernaviX/rollup-plugin-commonjs-broken-isomorphic-require

Bundle a module which checks if require is defined, and requires a module if it is.

Expected Behavior

Since the require statement will never be called in the browser, it should ideally be transpiled out. Since the original code worked in the browser, the new code should work too.

Actual Behavior

The require statement is converted into a static import. Even though the module is never actually used, it becomes a hard dependency and the file no longer works in the browser.

Relevant snippet of code:

// Add isomorphic support for TextDecoder
let TextDecoder = undefined;
if (typeof window === "object") {
  TextDecoder = window.TextDecoder;
} else if (typeof self === "object") {
  TextDecoder = self.TextDecoder;
} else if (typeof require === "function") {
  TextDecoder = require("util").TextDecoder;
}

This might be a duplicate of #342, I'm not sure.

[lukastaegert]

@rollup/plugin-commonjs does not support mixing the ES module format (which uses export) with the CommonJS module format (which uses require). As soon as an import or export statement is used, this plugin assumes this is an ES module and it will not be transpiled.

[SupernaviX]

If this file wasn't getting transpiled, it would work; i'm pretty sure the source code is a valid es module even though it "uses" require. Could there be a bug in the logic to skip es modules?

[lukastaegert]

It is a valid ES module, but in a valid ES module require is just a function without any meaning, and this is what happens. There is no specification of a module format that supports mixing those styles even though Webpack makes people believe there is. So in a sense you could call this a "Webpack style module". What keeps you from replacing your export statements with assignments to exports?

Also, how is this isomorphic if you expect export statements to work in your target environment? In browsers, you can only place this in a script tag if the type is module.

[lukastaegert]

Ah sorry, reading your issue again, maybe I slightly misunderstood the problem. The thing is, require is an unknown global function that might have side-effects. And even if it were not, there might be a side-effect when accessing a property on its return value. And even if this was not the case, you are assigning the return value to a variable you are using. This is actually not related to the plugin but how Rollup sees it. Not sure if there is a way to achieve what you want as you would need to tell Rollup in a way that you will never reach the last if case

[TrySound]

@lukastaegert Looks like the behaviour of detection was changed. Now this issue appears here and there because of it.
#304

[manucorporat]

I can confirm we can't longer compile stencil with rollup because the commonjs plugin is trying to wrap ESmodule code around the commonjs wrapper, leading to Error: 'import' and 'export' may only appear at the top level

[SupernaviX]

Ah sorry, reading your issue again, maybe I slightly misunderstood the problem. The thing is, require is an unknown global function that might have side-effects. And even if it were not, there might be a side-effect when accessing a property on its return value. And even if this was not the case, you are assigning the return value to a variable you are using. This is actually not related to the plugin but how Rollup sees it. Not sure if there is a way to achieve what you want as you would need to tell Rollup in a way that you will never reach the last if case

@lukastaegert I think this is achievable; I can see that the plugin is already replacing references to typeof module and such with "object". We tried using the replace plugin to change typeof require to "undefined" in FredKSchott/snowpack#286, and it convinced rollup to remove the unused import. Doing it from the commonjs plugin would be preferable, because that has access to the AST and can do it more robustly (and because this case would work out of the box). Would you accept a PR for that?

[lukastaegert]

A PR would surely be welcome but the question is what it should actually do. So if by default, typeof require is changed to "undefined", it would actually break UMD modules. What I am saying is, usually changing it to "function" is correct, which is then turned into a static import but removes the unneeded alternatives. What is the correct thing to happen here? Take TextDecoder from the global object? And why not the import?

[arlac77]

I can confirm we can't longer compile stencil with rollup because the commonjs plugin is trying to wrap ESmodule code around the commonjs wrapper, leading to Error: 'import' and 'export' may only appear at the top level

Same for me when using 'symbol-observable'

[!] Error: 'import' and 'export' may only appear at the top level
node_modules/symbol-observable/es/index.js (2:0)
1: /* global window */
2: import ponyfill from './ponyfill.js';

[SupernaviX]

Hmm, looking at the problem again I see your point, turning typeof require into "undefined" would fix my specific problem but cause weird issues in other places. I also realize I explained my issue poorly.

The file that's causing me trouble is a valid ES module, which works (without transpilation) in both the browser and Node 8+. It's trying to use a TextDecoder class, which is available as window.TextDecoder in the browser and require('util').TextDecoder in Node. If window.TextDecoder exists (i.e. if we're in the browser), it doesn't even try following the node codepath and require never gets called (or even accessed). In other words, this file only has a dependency on util when run in node.

My problem is that the commonjs plugin is transpiling that conditional require('util') into a static import util from 'util'. Even though the file will use window.TextDecoder in the browser, now it will try importing that node builtin no matter where it runs.

Tricking the commonjs transform tool into thinking that require was undefined solved the issue for me, because the block that actually called require got optimized out instead of turned into a static import. Thinking about it, a better solution would be if the system worked as you described earlier, where files with import or export statements weren't affected by the plugin at all; after all, the whole point of the code snippet I linked is that it should work in the browser without any extra modifications.

[lukastaegert]

The thing I overlooked is that my comment #348 (comment) is actually no longer valid because #206 changed semantics to always process modules if they contain global require calls.

So what we would need here would be a toggle to switch back to the old behaviour which was to not process any file that contains mixed ESM and CJS syntax. Would this work for you?@danielgindi as you are more intimate with that code, what do you think? Or maybe you have another idea?

[lukastaegert]

As such, it would actually be a duplicate of #342

[danielgindi]

The thing I overlooked is that my comment #348 (comment) is actually no longer valid because #206 changed semantics to always process modules if they contain global require calls.

So what we would need here would be a toggle to switch back to the old behaviour which was to not process any file that contains mixed ESM and CJS syntax. Would this work for you?@danielgindi as you are more intimate with that code, what do you think? Or maybe you have another idea?

I think that the latest PR should have fixed it. Should try and see. If all hell breaks loose sure we can revert to the old behavior where we don’t even look in ES module files- but that will break behavior where an ES module actually uses require.

We could also easily add a check for “typeof require”- which will make it much smarter, and will be very easy to implement.

[lukastaegert]

My understanding is that the latest PR just avoided adding a wrapper but would still bundle the file in question. If require is just ignored, it would work in the browser because even though the call is not removed, typeof require will avoid this case. Setting typeof require would of course be nicer but as I said, it could have the opposite effect if bundling the optional import is actually intended.

[danielgindi]

@lukastaegert Yes you raise a good point. We probably have no good way of knowing whether the user intend for the module to be transpiled for the browser (hence typeof require should resolve to undefined) or for a node env, or whether they actually want to keep the typeof require to keep the code isomorphic to some extend. So manual control over this can't be avoided, until everyone stops using these hacks of typeof require.

[danielgindi]

Well for this specific case the right path would be to use include/exclude to mark code that should not be transformed and code that should.
The fact that it worked previously when no require calls were transformed in ES modules - is just a lucky (unlucky?) accident. A combination of modules that were using UMD in just the right way to play nice with the specific situation.

We will have a kill switch for this soon anyway, so any breakage will be fixed. But people should really start sorting out their mess.

[SupernaviX]

If this code is inside of a dependency which is explicitly an ES module (i.e. package.json has a "module" field), does it make sense to automatically exclude it? Or would that mess with its (potentially non-ES-module) dependencies?