Accessing unknown globals inside a typeof guard shouldn't have side-effects

[Conduitry]

Feature Use Case

This Svelte issue. Svelte's internal library has code that looks like:

export let SvelteElement;
if (typeof HTMLElement !== 'undefined') {
	SvelteElement = class extends HTMLElement {
		// ...
	};
}

Since Rollup 1.21.0, this is being included in all bundles, whether or not they use SvelteElement, as the reference to the unknown global HTMLElement as the super class is assumed to possibly have the side-effect of throwing an error if it is not defined.

Feature Proposal

Code that references unknown globals inside of a typeof guard for that global shouldn't be assumed to have side effects.

[Conduitry]

I guess as part of this, I'm asking for the following code to also treeshake out:

if (typeof SomeUnknownGlobal !== 'undefined') { // or any other pure expression involving `typeof`
	// do nothing
}

Merely checking typeof isn't going to throw, even if it's undeclared.

[kzc]

The unknown global side effects PR is indeed the reason for the change in behavior, but not due to typeof specifically.

The actual issue is class C extends UnknownGlobal {}. Rollup used to drop this code if C were unused even though it would be an error in NodeJS:

$ echo 'class C extends UnknownGlobal {}' | node
[stdin]:1
class C extends UnknownGlobal {}
                ^
ReferenceError: UnknownGlobal is not defined

Likewise, as far as Rollup is concerned, HTMLElement is an unknown global and it is dependent on the JS runtime environment in which it is run.

Given:

$ cat test.js
if (typeof Unknown1 == "undefined") {}

if (typeof Unknown2 == "undefined") {
    console.log("Unknown2 is undefined");
} else {
    console.log("Unknown2 is defined");
}

class MyClass extends Unknown3 {}

let SvelteElement;
if (typeof HTMLElement !== 'undefined') {
    SvelteElement = class extends HTMLElement {}
}

using rollup v1.20.3:

$ dist/bin/rollup test.js -f es --silent
if (typeof Unknown2 == "undefined") {
    console.log("Unknown2 is undefined");
} else {
    console.log("Unknown2 is defined");
}

using rollup v1.21.2:

$ dist/bin/rollup test.js -f es --silent
if (typeof Unknown1 == "undefined") ;

if (typeof Unknown2 == "undefined") {
    console.log("Unknown2 is undefined");
} else {
    console.log("Unknown2 is defined");
}

class MyClass extends Unknown3 {}

let SvelteElement;
if (typeof HTMLElement !== 'undefined') {
    SvelteElement = class extends HTMLElement {};
}

Notice that the unused MyClass is now retained by the most recent version of Rollup due to extends Unknown3.

typeof Unknown1 was dropped by the old version of Rollup only because no code was dependent on the side effect free condition. Granted, having UnaryExpression#hasEffects return false for any typeof Identifier would be useful in its own right, but it wouldn't help in this case. The code for if (typeof Unknown == "undefined") foo(); else bar(); would still be retained in any case. Changing the logic of UnaryExpression#getLiteralValueAtPath to return "undefined" for the typeof unknown globals would break many polyfills.

What you could do is ignore any side effects from HTMLElement by using a pure annotation:

$ cat se.js 
let SvelteElement = /*@__PURE__*/ (() => class extends HTMLElement {
    // implementation of class SvelteElement
})();

$ dist/bin/rollup se.js -f es --silent
(!) Generated an empty bundle

or disable treeshake.unknownGlobalSideEffects.

[lukastaegert]

Created #3117 to address this

[Conduitry]

Thank you! I can confirm that the issue I was seeing in Svelte app bundles is fixed when using Rollup 1.21.4.

[bathos]

Since Rollup 1.21.0, this is being included in all bundles, whether or not they use SvelteElement, as the reference to the unknown global HTMLElement as the super class is assumed to possibly have the side-effect of throwing an error if it is not defined.

FWIW, referencing any binding in the global scope can have side effects. The typeof operator isn’t related to that — the side effect doesn’t need to be a ReferenceError. Var bindings at that scope are realized as properties of the global object.

Lots of builtin globals are implemented as accessors, and some of them really may have side effects. There are global vars(properties) which do or do not throw on reference(access) based on the nature of the document’s origin / iframe sandbox constraints / etc.