doesn't produce valid es3 javascript code

[sormy]

• Rollup Version: v0.66.4
• Operating System (or Browser): macOS
• Node Version: v11.3.0

How Do We Reproduce?

Rollup doesn't quote keys in export objects and that is causing some reserved keywords (like "default") to be used as object keys. That is not compatible with es3 (ie6/ie7).

Proposed fix is:

diff --git a/src/ast/variables/NamespaceVariable.ts b/src/ast/variables/NamespaceVariable.ts
index f1221f5a..f8a0dbde 100644
--- a/src/ast/variables/NamespaceVariable.ts
+++ b/src/ast/variables/NamespaceVariable.ts
@@ -88,7 +88,8 @@ export default class NamespaceVariable extends Variable {
                                }${_}}`;
                        }
 
-                       return `${t}${name}: ${original.getName()}`;
+                       // quote name to escape JS reserved words, for example, "default" in es3
+                       return `${t}'${name}': ${original.getName()}`;
                });
 
                const name = this.getName();

I can submit PR and fix unit tests if you are ok with this solution.

Another solution is to create new rollup options where target js version is specified and according to es3/es5/es6 version we could escape only required parameters. Will need to steal some code from babel in that case and dev effort will be much higher.

Expected Behavior

Ideally rollup bundle should be compatible with es3. It is minifier responsibility to remove extra quotation where it is needed.

Actual Behavior

Produces objects like { default: ... } in js source code and it is not parseable by IE7.

PS

I know that IE6/7 are dead on desktop but these browsers are still used on mobile devices operating Windows CE 6/7 (a lot of zebra hand scanners, for example). It is nice that with modern build tools we could write once and transpile code to multiple platforms but, unfortunately, current rollup version has this small compatibility issue that could be easily fixed.

[sormy]

Ping!

[sormy]

Ping!

[lukastaegert]

Hi @sormy,
sorry for not answering yet. I definitely see your issue and I see that Rollup could be in a good position to handle these use cases. However, since I am doing most of the heavy lifting for Rollup alone at the moment, I would only support a solution that can be maintained with minimal effort. Also, I am not happy with making the code worse for everyone else to support some specific use case, especially if the changed code will not even work reliably in the new situation. Because there are many more non-ES3 or even IE8 compatible code snippets, such us the use of Object.freeze for dynamic namespaces and I am sure there are others.

What I could see for Rollup is to support a compatibility mode for Rollup which would imply setting a compatibility target and change/omit some transformations based on that. But also note that we recently REMOVED an IE8 compatibility feature because it became unmaintainable, so to revert on that stance, a few things need to happen.

1. We NEED reliable tests that actually run code on those systems to ensure compatibility, and those tests need to run automatically via our CI (CircleCI). There is not way around this. Otherwise, we WILL break compatibility again in the future with some refactoring. And we have no easy way to address bug reports related to those features. Browserstack e.g. supports IE starting with version 6 and provides a free tier for OSS that we could apply for. Maybe we can get Karma to run on those systems?
2. I do not see myself doing this. I am doing most of the maintenance alone at the moment and see this as a feature that would only be attractive to a small fraction of our users so I would rather focus my energies on other things (there are quite a few big topics that many users would profit from). So this would need someone else to spearhead this. That being said, I would gladly provide support, feedback and reviews for anyone attempting this. We could even consider setting aside some funds from OpenCollective to support this work if this would help. Having browser based tests alone would already prove beneficial for many other things, e.g. we could start testing code splitting with real SystemJS or AMD runtimes.
3. It should ideally be addressed as a compatibility mode. Not sure what the options should be. The easiest maintainable ones would be to name them after the tested browsers, e.g. "IE6", "IE8" etc. Putting in some thought here could also help. There should not be too many different options as all need to be maintained.

[sormy]

First of all, thank you for your hard work to support rollup.

Integration tests running on real browsers is a nice feature to have however I don't see why this small fix can't be easily covered by unit tests.

Quoting the export name doesn't break upward compatibility, have no performance degradation, and doesn't not significantly contribute to bundle size (indention takes much more space than two extra characters per exported entry). If minification is used, these extra quotes will anyway gone. I think there is no any harm to keep it as default. However, I'm fine if it will be gated by config option, if you disagree.

Regarding overall compatibility of Rollup with ES3 (IE6, IE7, IE8). Rollup is very very very compatible based on my experience. This is the only incompatible thing I found in rollup core. Object.freeze() could be easily polyfilled. The only thing that can't be polyfilled in any way is Object.defineProperty() with getters/setters (and any related feature, like "decorators", since they use getters/setters too).

I have pretty big application that cross compiled to different platforms using different set of plugins. Currently I have to patch some plugins to make final bundle fully compatible with ES3. You will be probably surprised, but es3 keyword quoting is the only but typical topic of all these patches.

Babel with env preset targeting es3 and "transform-es3-member-expression-literals", "transform-es3-property-literals" can do all the work, however, wrapper code can't be processed by babel plugin.

Current incompatibilities:

• this proposal, 1 line fix that is easy to cover by unit test
• es3 support rollup-plugin-commonjs#364, reserved keyword "default", 1 line fix that is easy to cover by unit test (no reply)
• babel-helpers - es3 support babel/babel#9226, react helper with reserved keyword "for", 1 line fix that is easy to cover by unit test (got approval to make change in the code)

After these fixes (3 lines of code change in total) rollup with typical plugin set is fully compatible with es3.

I would like to contribute back es3 support, provide documentation for rollup for es3 if needed and cover by unit tests.

[sormy]

Ping!

Is this a such big deal to accept 3 line fix? I can submit PR, add unit tests. Just let me how would you like me to fix the issue. I can gate the feature by outputOption.

Keep in mind, Babel now has full es3 support on @babel/preset-env level and rollup/rollup-plugin-commonjs also accepted es3 comopatibility fix. This is THE ONLY ES3 rollup-incompatibility I'm aware of.

[lukastaegert]

I guess considering that we also have something like this in our interop wrapper, we might just as well just do your originally proposed change though I still find it wasteful to do it for any variable name, not just for "default". Are there others to be considered? Adjusting the existing tests should then probably be enough. Just be aware that this does not give you full ES3 compatibility in case your dynamic namespaces contain reassigned variables, in which case getters are used, and I do not feel very happy about reintroducing the legacy option we removed last year to achieve that.

So unless we have integration tests, it will be just guesswork.

[sormy]

How about using reserved-words to escape all es3 reserved words and keep others not touched? Do you agree to pull extra dependency? or I can hardcode es3 reserved words (easier, no dependencies).

[shellscape]

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

[lukastaegert]

#4215 Should finally fix this