chore(commonjs): transpile dynamic helper to ES5

[JLHwung]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

If maintainer insists I could port JLHwung/babel@434968d to rollup

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

Description

The let and const declarations in dynamic helper has caused @babel/standalone (built with rollup) crashed on ES5 environments. babel/babel#14112

In this PR we replace those declarations by variable declarations. I have looked through the code and don't see any required further adjustments. The dynamic helper now can be parsed in an ES5 engine.

As a bonus the new helper is also smaller because var is shorter than const.

An alternative solution is to export the commonjs helper id and then allow the helper to be transformed in plugin-babel. However, in this approach we lost control of the helper and it may introduce more issues due to user Babel configs.

[shellscape]

This really breaks convention in the repo and sibling plugins. Eventually we'd like to move this plugin to TypeScript, and that would mean you're back to square one with the issue in Babel. Agreed the fix is an important one, but this solution feels wrong to me.

[JLHwung]

Eventually we'd like to move this plugin to TypeScript, and that would mean you're back to square one with the issue in Babel

The helper is injected to userland so eventually we have to ship JavaScript in some way. If we migrate the helper to TS then we can set the output target to ES5.

Note that the HELPERS helper is ES5 anyway so I assume the helpers in commonjs plugin should at least be ES5, turns out the only ES6 feature is const/let so I replaced all of them.

[lukastaegert]

I also noticed this and actually included a similar change in #1038. Unfortunately, that one does not yet work well in watch mode, which is why it is still a draft.
@shellscape this does not have any implications on transforming to TypeScript as the code changes in question actually happen inside a big string literal (which may not be apparent from the diff), i.e. this is the code injected into the client bundle.
And it is usually a very good idea to make such code ES5 by default.

As I said, #1038 will make this unnecessary, but in the meantime, this should definitely be merged.

[shellscape]

@lukastaegert rare instance when we disagree :) The code is still managed as regular code in the plugin. Having something sitting within /src implies that it's source, and not already hand-curated build output, which is what this edit effectively does. A much more portable approach would be to separate it from /src or exclude it from the primary build step, and build it separately using a different set of options.

[JLHwung]

I can understand @shellscape 's concerns here. If we consider helper codes as part of source code (though wrapped in a big literal), replacing let/const by var is somehow degrading.

Now suppose we already build the helpers separately, we can revert this PR (if we still remember) or we can enable the prefer-const and no-var eslint rules so every helper will be subjected to the same quality requirements and fixing those lint errors effectively revert this PR.

I think we are on the same page: 1) the helper code should be managed as a source instead of a big string. 2) the helper code should run on ES5 environments. Now before we separate the helper build step, I think it is acceptable to temporarily transpile helpers to meet 2) and eventually we settle down a way to 1).

[lukastaegert]

I am not sure we are on the same page here as the helper code is and always has been a hand-curated string of code embedded in a constant in the regular code. I am not sure though if it would be worthwhile to add a second build step to just generate the helper code. It is not a lot of code and hand-curating something that will show up verbatim in everyone's bundles feels worthwhile to me. Especially since I have already rewritten most of it in #1038

[lukastaegert]

If someone would set up this transpilation, that would also work, admittedly maybe I am just lazy.

[shellscape]

@JLHwung on reflecting on the comments here, I think we're good to merge. please give a look at the conflict and we'll get this in.

[JLHwung]

@shellscape It's been quite a while, I completely forget how I manage to update snapshot in the last commit.

[shellscape]

No worries. I'd delete the snap and accompanying .md file, and rerun pnpm test inside the plugin's directory.

[shellscape]