Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The cjs bundle is not fully es5 compatible #901

Closed
bartsidee opened this issue Feb 17, 2021 · 8 comments · Fixed by #957
Closed

The cjs bundle is not fully es5 compatible #901

bartsidee opened this issue Feb 17, 2021 · 8 comments · Fixed by #957
Milestone

Comments

@bartsidee
Copy link

bartsidee commented Feb 17, 2021

The cjs bundle generated by tsdx is not fully es5 compatible

npx es-check es5 ./dist/redux-toolkit.cjs.development.js

results in the below error related to the const references found

ES-Check Error:
          ----
          · erroring file: dist/redux-toolkit.cjs.development.js
          · error: SyntaxError: The keyword 'const' is reserved (1412:0)
          · see the printed err.stack below for context
          ----

          SyntaxError: The keyword 'const' is reserved (1412:0)

this is caused by the below reference in redux-toolkit.cjs.development.js

const _iteratorSymbol = /*#__PURE__*/ typeof Symbol !== "undefined" ? (Symbol.iterator || (Symbol.iterator = Symbol("Symbol.iterator"))) : "@@iterator";

const _asyncIteratorSymbol = /*#__PURE__*/ typeof Symbol !== "undefined" ? (Symbol.asyncIterator || (Symbol.asyncIterator = Symbol("Symbol.asyncIterator"))) : "@@asyncIterator";

which looks like a reference from symbol-observable package used by redux. It is however not referenced by the redux-toolkit generated cjs bundle. This is likely also the reason why this is not an issue for the ./dist/redux-toolkit.cjs.production.js as the minifier would removing the dead code.

A temporary workaround for us is to manually create an alias to ./dist/redux-toolkit.cjs.production.js however it would be great to get this fixed in the redux-toolkit pipeline itself.

Solution
I was able to solved this in the build pipeline by:

  • upgrading tsdx to the latest version (0.14.1)
  • and update the nested dependency of @rollup/plugin-commonjs to 12.0.0.

I just send a PR to tsdx project to ask if they would consider a version bump of the commonjs plugin jaredpalmer/tsdx#975

@phryneas
Copy link
Member

phryneas commented Feb 17, 2021

ES5 is a 12 year old standard.
ES6 is 6 years old by now.
The current JavaScript standard is ES11 or ES12.

I see the reasoning to still be compatible with Internet Explorer, but why do you need to be compatible with stuff that is even more ancient than that?

@bartvandenende-wm
Copy link

we are not targeting 'traditional' desktop browsers, our projects target environments on embedded hardware where the browser version is linked to the device firmware and is not automatically updated.

@phryneas
Copy link
Member

phryneas commented Feb 17, 2021

Then you should probably transpile all your dependencies down to that level as part of your build process anyways.
Most libraries nowadays target something like "last 2 maintained browser versions + maybe ESR versions" using a tool like babel-env, as the difference in size between ES5 and that is very significant. So I imagine you won't have those problems only with RTK, but with lots of dependencies.

Honestly, I see us going that route with RTK in the near future as well (maybe with an extra build tailored for some known-to-be-old react native runtimes), much more likely than tweaking the build to support even more restricted environments.

@markerikson
Copy link
Collaborator

I can see both sides here.

On the one hand, there's a reasonable assumption that the transpiled builds fully target ES5, no ES6 syntax at all, and it sounds like this isn't a huge change for us to make - mostly updating our existing build tooling.

On the other hand, the particular platform does sound like a rare edge case, and the ecosystem is trying to lean towards a focus on modern browsers.

@bartsidee Given that you've done the research work on this, could you try putting up a PR to update our build environment and see how that builds?

@phryneas
Copy link
Member

The problem is that we'll be merging in RTK-Query in a few months down the line, which will mean a rehaul of our complete build system, away from tsdx.
And there we've already seen (rtk-incubator/rtk-query#109, rtk-incubator/rtk-query#141) that even just going from the ESM builld (babel-env with 'defaults', 'not IE 11', 'maintained node versions', 10.84 KB) to the CJS build (babel-env with 'defaults', 16.37 KB) is a 60% size increase.
So if we were to go with even older targets there, that might place a big additional burden on our general user base to support extreme edge cases.

But I've given that build there a test run and it seems like npx es-check es5 reports no errors on it, so it might be fine without further size increase?

Either way, I'm a bit hesitant to put any more work into the current build system, be it from us or from @bartsidee, just becaue we know we'd probably use it only for one minor release (if we have another release in-between at all) and then scrap it.

So, @bartsidee: If you want to go for it, I guess I won't stop you as long as we don't really increase significantly in size, but your work might be replaced by something else pretty shortly down the line. I really think it will benefit you more if you put the same time & energy into transpiling your node_modules for the time being.

@bartsidee
Copy link
Author

bartsidee commented Feb 18, 2021

thanks @phryneas, we mainly aim for es5 'syntax' compatibility for the cjs bundle which is already supported.

To provide some additional background, our applications have lots of components split across many teams and projects. So for performance reasons, we've agreed upon a standard set of polyfills that everyone uses. The polyfills are loaded manually, and transpilation is performed by the TypeScript compiler instead of Babel.

The core issue related to rollup, is as I understood fixed in the next major release of tsdx per jaredpalmer/tsdx#889 However if the redux-toolkit tool chain will change in the near future and ejs bundles are es5 safe that could work as well.

Until then we can use the temporary workaround to alias the import to the production cjs bundle.

@markerikson
Copy link
Collaborator

It does look like we're going to be redoing the build system in preparation for 1.6 and the RTK Query functionality. Can you keep an eye on the repo and help us get that tweaked when we do so?

@markerikson markerikson linked a pull request Apr 1, 2021 that will close this issue
@markerikson markerikson added this to the 1.6 milestone Apr 1, 2021
@markerikson
Copy link
Collaborator

Looks like #957 should fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants