Getters/setters should be removed somehow

[IvanSanchez]

In Leaflet, we've got a bit of code like:

export var disableTextSelection;
export var enableTextSelection;
if ( /* browser has some feature */ ) {
    disableTextSelection = function () {...};
    enableTextSelection = function () {...};
} else {
    disableTextSelection = function () {...};
    enableTextSelection = function () {...};
}

Normally rollup will take all the exports and wrap them in a Object.freeze(), like:

var MyModule = Object.freeze({
    setPosition: setPosition,
    getPosition: getPosition,
    get disableTextSelection () { return disableTextSelection; },
    get enableTextSelection () { return enableTextSelection; },
    disableImageDrag: disableImageDrag,
    enableImageDrag: enableImageDrag
});

For some reason I don't fully understand (related to the fact that the exports are variables and not explicit functions), they're just a getter.

Now, when running rollup-plugin-es3, the resulting code will be as follows:

var MyModule = {
    setPosition: setPosition,
    getPosition: getPosition,
    get disableTextSelection () { return disableTextSelection; },
    get enableTextSelection () { return enableTextSelection; },
    disableImageDrag: disableImageDrag,
    enableImageDrag: enableImageDrag
};

...and that will trigger a parse error, even in ES5 and ES6 environments. 😞

cc @mourner - this is blocking IE8 support in Leaflet/Leaflet#4989
cc @Rich-Harris - maybe the getters/setters issue is a core rollup issue

[mourner]

@IvanSanchez instead of this plugin, as far as I remember, rollup recently landed a legacy option for this.

[futurist]

@IvanSanchez Please see this PR rollup/rollup#1068

I've merged this plugin into legacy option, so this plugin is no needed after this.

About the getter issue, I think it's the AST problem to identify the isReassigned for the variable reference, you can check below line:

https://github.com/rollup/rollup/blob/master/src/Declaration.js#L82

[IvanSanchez]

@futurist Thanks for the info!

I'll keep an eye on that PR then.

I assume that this plugin will be deprecated eventually?

[futurist]

I assume that this plugin will be deprecated eventually?

For a long term, YES.
But after the PR accepted, and after the legacy option covered ES3, I'll continue with this work.

[derrickb]

@futurist there doesn't appear to be a legacy option in Rollup anymore. Can this request be reopened? I found it by googling a way to remove setters/getters from Rollup output. Thank you.

[sormy]

There is no way to remove or transpile getters/setters to ES3 and any feature that is relying on them (like decorators). Any library that depends on Object.defineProperty() with getter/setter won't run on ES3. For these libraries the only way is to fork them and provide ES3 compatible implementation.

However, most libraries have similar older version that doesn't use modern getters/setters or decorators.

You could easily polyfill Object.freeze() as noop and Object.defineProperty() for cases when get and set are not passed to property descriptor even without this plugin. es5-shim and es5-sham have them.

However, rollup and bunch of base plugins like commonjs have an issue when named exports are not escaped and es3 can't event parse Symbol.for or module.default since for and default are reserved keywords in es3. You could read more here (there are workarounds): rollup/rollup#2591

[derrickb]

@sormy yes but in this case, the request is to remove getters/setters when there weren't any to begin with, and aren't necessary for the outputted code to function (eg. from the OP, a getter that just returns the property...it would have been fine as a normal property without the getter).

[sormy]

babel can exclude Object.defineProperty() for regular properties if loose transformation is used. I don't believe that is possible to cleanup code from extra Object.defineProperty() without deep static code analysis since property descriptor could be programmatically constructed during runtime. Much easier is to add Object.defineProperty() ponyfill but it could be done without rollup.