Treeshaking Removes Critical React DOM Code

[spencer516]

• Rollup Version: 1.24.0
• Operating System (or Browser): Mac OSX (10.14.6)
• Node Version: v10.15.3

How Do We Reproduce?

A minimal reproduction repo can be found here: https://github.com/spencer516/react-rollup-bug

(I could not recreate the same behavior in the rollup repl — not sure if it's on 1.24 or not).

Expected Behavior

When using the Production/Minified version of React DOM in an app that is bundled and treehsaken (treeshook?) by Rollup, I would expect the application to behave identically before and after the tree shaking. Specifically to React DOM: I expect it to insert the elements I render.

Actual Behavior

Rollup is removing small, but critical, parts of React DOM causing the application not to render and not to throw any runtime errors. The parts that are removed appear to be related to all of the DOM Node Inserting here.

A few notes:

1. The app works normally when treeshaking is disabled
2. The app works normally on v1.23.1 and earlier
3. This only happens with the minified production version of React DOM (./react-dom/cjs/react-dom.production.min.js)

To isolate the difference of treehsaking React DOM in 1.23 vs 1.24, I built my app in both version, ran Prettier to format the minified code, and ran a diff. The additional code removed can be seen here: spencer516/react-rollup-bug@dist-1.23...dist-1.24

I suppose it's possible this is also a React DOM bug (perhaps their minified code is invalid in some way?) — I'm happy to open an issue there, too, if it helps.

[lukastaegert]

Thanks for spotting, this is very helpful, I think I see what is going on. Will dig into this soon.

[lukastaegert]

Here is a condensed version of the problem in the REPL: https://rollupjs.org/repl/?version=1.24.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmElM0ElMjAlN0IlNUNuJTIwJTIwZm9yJTIwKGNvbnN0JTIweCUyMG9mJTIweSklMjAlN0IlNUNuJTIwJTIwJTIwJTIwaWYlMjAoY29uZGl0aW9uKHgpKSUyMCU3QiU1Q24lMjAlMjAlMjAlMjAlMjAlMjBicmVhayUyMGElM0IlNUNuJTIwJTIwJTIwJTIwJTdEJTVDbiUyMCUyMCU3RCU1Q24lMjAlMjB0aHJvdyUyMG5ldyUyMEVycm9yKCklM0IlNUNuJTdEJTVDbiU1Q25jb25zb2xlLmxvZygndGhpcyUyMGlzJTIwbm90JTIwZGVhZCUyMGNvZGUnKSUzQiU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlc20lMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

[lukastaegert]

I think the long and short is that I did not consider that labeled statements do act as a means of optional control flow and therefore errors inside labeled statements need to be handled similar to errors inside if-statement branches.

[spencer516]

Awesome! (I just learned today that labeled statements exist. 🙃 Neat!)

Thanks for looking into this.

[lukastaegert]

😉 I think I'll be able to release a fix tomorrow.

[jackturnbull]

A little thank you for creating the issue (always helps to know I'm not going mad.) I'd noticed it too but was trying to figure out whether it was rollup or sentry in this instance. The fix for the time being of course is to remain on 1.23.1 for now - thanks also for addressing the issue so quickly :)

[lukastaegert]

Fix at #3170. Interestingly, fixing this made the logic simpler rather than more complicated, so I snuck in another feature as well which is to remove unused labels.

[lukastaegert]

BTW I am very happy that there are people compiling React using the latest versions of Rollup. There is a long-standing PR to finally get React to upgrade to the 1.x versions of Rollup but apparently it is not moving forward, which makes me quite sad: facebook/react#15037

[jbt]

I just came across this issue after running into something similar myself with React DOM breaking in 1.24 which isn't fixed in 1.25. I'm not sure if it's another case of this issue or something different, but it appears to be related to code being treeshaken inside the commitAttachRef function. I'm just working on putting together a minimal test case now to see if it's similar or a completely different issue

[lukastaegert]

A diff between the bundled code with 1.23 and 1.25 would be helpful. By now I have an idea that there might be another situation I may have handled wrongly which is a do..while loop containing a conditional break and an unconditional throw or return, but there might be more, which is why I am very interested in this. Will probably not be able to put out something new before Monday, though.

[jbt]

I managed to dig into the ReactDOM bugs with 1.25 and it looks like it's different behaviour from this issue, so I opened a new one: #3177