rolling up some ES module code as a single ES module yields a very broken module...

[Pomax]

• Rollup Version: 1.27.0
• Operating System (or Browser): macos + firefox 72/chrome 78
• Node Version: 12.10.0

How Do We Reproduce?

1. Grab https://github.com/Pomax/Font.js/tree/89abc43f8671d6de1fe4e55d9ce92f63f33cfe99
2. run an http server in the root dir, and open the index.html in the browser on the http protocol
3. things work. There are no errors in the console in Chrome, and three errors in Firefox (due to it lacking support for something used in the variable otf fonts).
4. roll up Font.js using rollup --format=esm Font.js > Font.rolled.js and edit index.html to use this rolled up script instead.
5. Reload the browser
6. Things have gone horribly wrong, looking at the console O_o

Expected Behavior

Identical behaviour in steps 2/3 and steps 5/6

Actual Behavior

steps 5/6 show things having broken considerably.

I don't even know how to go about trying to reduce things here.

[kzc]

No need for adjectives like "very broken" and "horribly wrong". It's insulting to the authors of the project. By definition any bug will produce an incorrect result. Stating the facts is sufficient.

If you use --no-treeshake it'll likely work.

The input code is creating getters dynamically and Rollup appears to be unaware of that:

class Parser {
    constructor(name, dict, dataview) {
        this.name = name;
        this.length = dict.length;
        this.start = dict.offset;
        this.offset = 0;
        this.data = dataview;

        [   `getInt8`,
            `getUint8`,
            `getInt16`,
            `getUint16`,
            `getInt32`,
            `getUint32`,
            `getBigInt64`,
            `getBigUint64`
        ].forEach(name => {
            let fn = name.replace(/get(Big)?/,'').toLowerCase();
            let increment = parseInt(name.replace(/[^\d]/g,'')) / 8;
            Object.defineProperty(this, fn, {
                get: () => this.getValue(name, increment)
            });
        });
    }
...
    getValue(type, increment) {
        let pos = this.start + this.offset;
        this.offset += increment;
        try {
            return this.data[type](pos);
        } catch (e) {
            console.error(`parser`, type, increment, this);
            console.error(`parser`, this.start, this.offset);
            throw e;
        }
    }

If you diff the output without/with tree shaking you can see that it drops statements like this:

@@ -1282,7 +1266,6 @@
         this.flavor = p.uint32;
         this.length = p.uint32;
         this.numTables = p.uint16;
-        p.uint16;
         this.totalSfntSize = p.uint32;
         this.majorVersion = p.uint16;
         this.minorVersion = p.uint16;
@@ -1353,7 +1336,6 @@
         this.length = p.uint32;
 
         this.numTables = p.uint16;
-        p.uint16; // why woff2 even has any reserved bytes is a complete mystery. But it does.
 
         this.totalSfntSize = p.uint32;
         this.totalCompressedSize = p.uint32;

Rollup thought those statements were side-effect-free and could be safely removed. That is not the case. Arguably they should be retained if propertyReadSideEffects is in effect, which is the default - but that's debatable.

[Pomax]

No offense intended, nor malice behind the post; they were words used in surprise, not anger.

Thank you for the reply.

[shellscape]

Things have gone horribly wrong, looking at the console O_o

I for one took "horribly wrong" in jest as it was followed by the O_o, and giggled when I read it.

Please let us know if @kzc's suggestion works.

[Pomax]

Yep, with the addition of --no-treeshake Font.js now rolls ups to a single file that works as well in the browser as the type="module" version.

It might be worth adding a note to the "Treeshaking" section in the readme that this will probably lead to a broken rolled up file if your code makes use of Object.defineProperty? But that would probably be a separate issue (let me know if you'd like me to file that).

[kzc]

As it so happens, even if the getters were declared in the class without Object.defineProperty it would not have worked due to this known bug:

• Getters not applying in side-effect detection Getters not applying in side-effect detection #2219

Rollup appears to assume that all getters are side effect free if tree shaking is enabled.

$ cat getter.js 
var result = "FAIL";
class C {
    get x() {
        result = "PASS";
    }
}
(new C).x;
console.log(result);

$ cat getter.js | node
PASS

$ dist/bin/rollup -v
rollup v1.27.0

$ dist/bin/rollup getter.js -f es --silent | node
FAIL

$ dist/bin/rollup getter.js -f es --silent
var result = "FAIL";
console.log(result);

@lukastaegert Other than disabling tree shaking altogether, is there a setting for propertyReadSideEffects that will allow code with getters with side effects to work correctly?

[lukastaegert]

Not yet, but it might be interesting to implement as a stop-gap solution, e.g. make every property access a side-effect with propertyReadSideEffects: always.