Rollup v0.56.5 breaks bundles where there are two (or more) classes defined with the same name

[ilmiont]

Please refer to the repro here: https://github.com/ilmiont/rollup-v0.56.5-test

The repro-v0.56.4.js was compiled in Rollup v0.56.4 and runs fine in current browsers (Chrome latest, Firefox latest).

The repro-v0.56.5.js was compiled from the same source in Rollup v0.56.5, and does not run in browsers, as it throws an Uncaught ReferenceError: Model is not defined.

This is caused by the change in Rollup v0.56.5 due to #2025 which creates the following in the bundle:

let Model$1 = class Model extends Model { }

Maybe it seems like an edge case that there are two classes with the same name (as written) in the project, but I really don't think it is. In my scenario, I've got an ecosystem of libraries with components that extend each other to add additional functionality.

The end result is, that in an app I'm working on, a search for "class Model" or "class Controller" definitions in the unbundled source will result in several matches, which wasn't an issue in Rollup v0.56.4 because it would just append $x to the end of each class name (where x is obviously an integer incremented for each new class definition of this name).

The new syntax in Rollup v0.56.5 means builds of this app are currently completely broken, and I don't think there's a way to mitigate this without me renaming all my classes. Sticking with Rollup v0.56.4 for now until this behaviour is confirmed.

[guybedford]

REPL case - https://rollupjs.org/repl?version=0.56.5&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyME1vZGVsQmFzZSUyMGZyb20lMjAlNUMlMjIuJTJGTW9kZWxCYXNlLmpzJTVDJTIyJTNCJTVDbiU1Q25jbGFzcyUyME1vZGVsJTIwZXh0ZW5kcyUyME1vZGVsQmFzZSUyMCU3QiU1Q24lNUNuJTVDdGNvbnN0cnVjdG9yKCklMjAlN0IlNUNuJTVDdCU1Q3RzdXBlcigpJTNCJTVDbiU1Q3QlNUN0Y29uc29sZS5sb2coJTVDJTIySSdtJTIwdGhlJTIwbW9kZWwuJTVDJTIyKSUzQiU1Q24lNUN0JTdEJTVDbiU1Q24lN0QlNUNuJTVDbnZhciUyMG1vZCUyMCUzRCUyMG5ldyUyME1vZGVsKCklM0IlMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyTW9kZWxCYXNlLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNsYXNzJTIwTW9kZWwlMjAlN0IlNUNuJTVDbiU1Q3Rjb25zdHJ1Y3RvcigpJTIwJTdCJTVDbiU1Q3QlNUN0Y29uc29sZS5sb2coJTVDJTIySSdtJTIwdGhlJTIwbW9kZWwlMjBiYXNlLiU1QyUyMiklM0IlNUNuJTVDdCU3RCU1Q24lNUNuJTdEJTVDbiU1Q25leHBvcnQlMjBkZWZhdWx0JTIwTW9kZWwlM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyY2pzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

Thanks for posting, it seems this is an edge case of #2025 where for some mysterious reason the JS spec has decided that the class name itself exists supersedes in the super expression scope even if it is a class expression.

The fix here will likely be to explicitly check if the class name itself is used as a non-deshadowed binding in the super expression and then to skip this renaming trick in that case!

//cc @NaridaL

[lukastaegert]

The fix here will likely be to explicitly check if the class name itself is used as a non-deshadowed binding in the super expression and then to skip this renaming trick in that case!

This will also affect references inside class methods.:
https://rollupjs.org/repl?version=0.56.5&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMFNoYWRvd2VkTW9kZWwlMjBmcm9tJTIwJTVDJTIyLiUyRk1vZGVsQmFzZS5qcyU1QyUyMiUzQiU1Q24lNUNuY2xhc3MlMjBNb2RlbCUyMCU3QiU1Q24lNUN0Y29uc3RydWN0b3IoKSUyMCU3QiU1Q24lNUN0JTVDdHN1cGVyKCklM0IlNUNuJTVDdCU1Q3Rjb25zb2xlLmxvZyglNUMlMjJJJ20lMjB0aGUlMjBtb2RlbC4lNUMlMjIpJTNCJTVDbiU1Q3QlNUN0U2hhZG93ZWRNb2RlbC5sb2coKSUzQiU1Q24lNUN0JTdEJTVDbiU3RCU1Q24lNUNudmFyJTIwbW9kJTIwJTNEJTIwbmV3JTIwTW9kZWwoKSUzQiUyMiU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJNb2RlbEJhc2UuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyY2xhc3MlMjBNb2RlbCUyMCU3QiU1Q24lNUN0Y29uc3RydWN0b3IoKSUyMCU3QiU1Q24lNUN0JTVDdGNvbnNvbGUubG9nKCU1QyUyMkknbSUyMHRoZSUyMHNoYWRvd2VkJTIwbW9kZWwuJTVDJTIyKSUzQiU1Q24lNUN0JTdEJTVDbiU1Q3QlNUNuJTVDdHN0YXRpYyUyMGxvZygpJTIwJTdCJTVDbiU1Q3QlNUN0Y29uc29sZS5sb2coJTVDJTIybG9nJTVDJTIyKSUzQiU1Q24lNUN0JTdEJTVDbiU3RCU1Q24lNUNuZXhwb3J0JTIwZGVmYXVsdCUyME1vZGVsJTNCJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmNqcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

I guess we will need to find a way to mark the class id as used when deshadowing variables.

[guybedford]

Ahh, right that is definitely a problem then as these deshadowing cases all effectively invalidate the optimization.

I think we should revert #2025 then at least until a new approach can be found.