Issues in React 17 with terser and minifiers (rewrite property access using dot notation) (useInsertionEffect)

[hezi-bigpanda]

In the following PR:
https://github.com/emotion-js/emotion/pull/2651/files

was suggested a fix the following issue:
#2649

It is a great mitigation but some tools tools are rewriting it again to dot notation (for instance, terser by default)
I needed as mitigation to disable this feature, however, I think it needs to be approached better at emotion.

I will do a PR with a suggestion to fix, but maybe you would like to approach it yourself.

I think the ordinal intention is problematic since ecmascript is a static module system and you want to use it as dynamic.

@Andarist I am tagging you because you found the issue and maybe you will have an idea

[Andarist]

Could you prepare a full runnable repro case for the problem though? I would like to check out how different tools interact in your setup

[hezi-bigpanda]

@Andarist , please paste following code:
var m = [];
var t = m['f' + 'y'];
console.log(t);

Here:
https://xem.github.io/terser-online/

Look at the output and you will see my point

[hezi-bigpanda]

I suggest to replace it to something like:
const useInsertionEffect = React['useInsertion'.concat('Effect')]
This way, terser can't statically minify it. Of course, it is not semantically equal since string.prototype.concat might be overridden.

I think the best would be to avoid this feature completely, but I guess it is something you won't accept

What do you think ? I will do a PR after I will test it

[Andarist]

I know how Terser can optimize this - I just wonder what's your exact build pipeline that makes this a problem. Like - what happens with the Terser's output in your case? Is it handed to a bundler? Is it executed directly in a browser?

[hezi-bigpanda]

Ok then
I use react-select which uses emotion as a dependency (not peerDependency) in a component library project that I created.
I bundle it using rollup with terser plugin. I publish the output as npm package and consume it in client projects (react is an external dependency of the bundle). Issue happens only if client uses React < 18.

it is being generated by terser to something like this: pw=r.useInsertionEffect?r.useInsertionEffect:d
Without terser plugin code matches the source code of emotion.
By configuring terser with evaluate:false issue is being mitigated for me

[Andarist]

My usual recommendation is to not bundle your dependencies, if your client decides to use Emotion on their own in their projects they might end up with hard to debug problems. Any particular reason why do you chose this approach?

[hezi-bigpanda]

Good point.

I can't assume that the application client bundles itself, so there is a performance consideration.
in addition, for the usage of plugins like svg loader

I believe for internal libraries it is a reasonable approach while for public libraries it might be better to just transpile like you suggest.

Obviously, frameworks like react and react-dom are not part of the bundle and being set as externals.

[hezi-bigpanda]

@Andarist What did you decide ? well, as mentioned I mitigated the issue so as far as I concerned you can close the issue , however, I am pretty sure I will not be the last so I suggest you to rethink of it.
I wanted to do a PR but permissions are required for it.

[TheSisb]

Hi @Andarist, we're encountering that issue as well on the Paste design system.

Our design system has a package called @twilio-paste/styling-library which wraps Emotion and Styled-system, and then sprinkles some minor changes on top. This styling library is a hard dependency for system; our consumers don't have a separate emotion install. This structure allows us to change the underlying nuts and bolts of our UI stack without having to dig into different teams' code, or having to wait for them to do upgrades themselves. We can vet and upgrade dependencies transparently with (usually) a patch change on our side.

Since React is a dependency we don't wrap and manage, we are still having to support v16 and v17. And so, during a routine Emotion version upgrade, we encountered the same issue as @hezi-bigpanda:

Failed to compile.

Attempted import error: 'useInsertionEffect' is not exported from 'react' (imported as 'Dr').

I noticed that our development build worked just fine, and that it would fail on prod builds. It seems ESBuild compiles both the new version and the old version:

var useInsertionEffect5 = React3["useInsertionEffect"] ? React3["useInsertionEffect"] : function useInsertionEffect6(create) {

into

var Wt=Dr.useInsertionEffect?Dr.useInsertionEffect:ln

---

To any readers, our workaround has been to downgrade emotion to before this hook started being used:

"@emotion/react": "11.7.1",
"@emotion/styled": "11.6.0",

We don't know when we will be able to drop support for React v16/17. Unless a fix is added into Emotion upstream we will either be blocked on that version or have to run a post-build script to update the problematic reference.

Thanks for your consideration.

[Andarist]

The problem in here seems to originate in the fact that you minimize your library by default. In general, I think this is a bad practice because it makes the debugging experience in development much worse. For a person who regularly dives into node_modules to explore and fix bugs, like me - this kind of thing is utterly annoying and gets in the way.

I, of course, won't be arguing about this here too much - but regardless of the reported issue, I would like to encourage you to reconsider this choice. I see that you also provide .debug.js files but I don't think this makes it that much better because I would have to know how to use those files instead of the default ones, maybe your process is to just copy-paste its content to the default location but to do that I would have to first learn that there is a .debug.js file in place and do a minor FS content dance just to get start debugging, with caching systems etc it might be even more problematic because it might require me to either restart the dev server or clear the on-disk cache. Such a process just comes with a lot of friction.

Either way - I don't think this is "fixable" on our side because it's not really a bug in its classic sense and we need to use this effect conditionally for the improved support of React 18. We provide files written in a way that is compatible with all the tooling that consumes them (we didn't have any reports about this being a problem anywhere), the problem is that you are acting as a middleman here and you change what we have intentionally written in a certain way.

That being said - there are definitely different ways to prevent constant folding in minifiers so I could accept a PR that would change this pattern to something else, something that would work better with Terser and esbuild, and something that would preserve the original shape of the code. I'm just afraid that I don't have time to play with this myself

[TheSisb]

Thanks for the reply @Andarist,

The problem in here seems to originate in the fact that you minimize your library by default.

This is an interesting point. Out of curiosity, wouldn't shipping unminified builds just bounce the issue to our consumer's build?

In general, I think this is a bad practice because it makes the debugging experience in development much worse. For a person who regularly dives into node_modules to explore and fix bugs, like me - this kind of thing is utterly annoying and gets in the way.

We've had to make the trade-off of slight DX degradation here in favor of improved UX for Twilio customers. As you discovered, most people who dig into node_modules also find our debug files and figure out next steps themselves. Not ideal, but the tradeoffs for our use-case play out this way unfortunately.

Either way - I don't think this is "fixable" on our side because it's not really a bug in its classic sense and we need to use this effect conditionally for the improved support of React 18.

Yeah, that's what I figured as well.

That being said - there are definitely different ways to prevent constant folding in minifiers so I could accept a PR that would change this pattern to something else

I'll try to play the minifier side-stepping game. If I figure it out I'll open a PR!

I really appreciate your thoughts on all this. Thank you so much for all the care you put into your work!

[Andarist]

This is an interesting point. Out of curiosity, wouldn't shipping unminified builds just bounce the issue to our consumer's build?

No, it shouldn't because your unminified code should roughly preserve the original shape of this code and final consumers' tooling would know what to do with it.

We've had to make the trade-off of slight DX degradation here in favor of improved UX for Twilio customers.

How does this help your customers though? Can you minify your bundles significantly better than your consumers? They will have a minifier in their build pipeline anyway, right?

I'll try to play the minifier side-stepping game. If I figure it out I'll open a PR!

I think there is also an argument to be made here to ask Terser and esbuild to skip constant folding when accessing a module namespace's property~. They know that the binding is a namespace object and not a regular object and they are changing it to a code shape that has some different traits so it's potentially an unsafe operation.

[TheSisb]

No, it shouldn't because your unminified code should roughly preserve the original shape of this code and final consumers' tooling would know what to do with it.

Hmm. Ok, I may consider this then.

How does this help your customers though? Can you minify your bundles significantly better than your consumers? They will have a minifier in their build pipeline anyway, right?

Would you believe me if I said not everyone minified in their build pipelines? 😰 I wish I had more power over it, but it's been easier for us to just ship it minified for the last ~2+ years. It's one of those short term solutions that just stuck around. I'll revisit it again internally.

think there is also an argument to be made here to ask Terser and esbuild to skip constant folding when accessing a module namespace's property~. They know that the binding is a namespace object and not a regular object and they are changing it to a code shape that has some different traits so it's potentially an unsafe operation.

🤷 I'll try to open an issue on the ESbuild repo for their thoughts. Good tip!

[Andarist]

I've created a PR that might allow you to workaround this issue: #2867