-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
ES5 is a 12 year old standard. 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? |
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. |
Then you should probably transpile all your dependencies down to that level as part of your build process anyways. 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. |
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? |
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. But I've given that build there a test run and it seems like 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. |
thanks @phryneas, we mainly aim for es5 'syntax' compatibility for the 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 Until then we can use the temporary workaround to alias the import to the production cjs bundle. |
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? |
Looks like #957 should fix this. |
The cjs bundle generated by
tsdx
is not fully es5 compatiblenpx es-check es5 ./dist/redux-toolkit.cjs.development.js
results in the below error related to the
const
references foundthis is caused by the below reference in
redux-toolkit.cjs.development.js
which looks like a reference from
symbol-observable
package used byredux
. 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:
tsdx
to the latest version (0.14.1)@rollup/plugin-commonjs
to12.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#975The text was updated successfully, but these errors were encountered: