rollup icon indicating copy to clipboard operation
rollup copied to clipboard

Class names are mangled, breaking runtime behavior that relies on them.

Open ascott18 opened this issue 3 years ago โ€ข 9 comments

Rollup Version

2.79.0

Operating System (or Browser)

Any

Node Version (if applicable)

NA

Link To Reproduction

https://rollupjs.org/repl/?version=2.79.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMFBhcmVudENvbXBvbmVudCUyMGZyb20lMjAnLiUyRnBhcmVudC1jb21wb25lbnQuanMnJTNCJTVDbm5ldyUyMFBhcmVudENvbXBvbmVudCgpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIycGFyZW50LXNjcmlwdC5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJpbXBvcnQlMjBDaGlsZENvbXBvbmVudCUyMGZyb20lMjAlNUMlMjIuJTJGY2hpbGQtY29tcG9uZW50LmpzJTVDJTIyJTNCJTVDbmxldCUyMFBhcmVudENvbXBvbmVudCUyMCUzRCUyMGNsYXNzJTIwJTdCJTVDbiUyMCUyMGNvbnN0cnVjdG9yKCklMjAlN0IlNUNuJTVDdCU1Q3Rjb25zb2xlLmxvZyh0aGlzLmNvbnN0cnVjdG9yLm5hbWUpJTNCJTVDbiU1Q3QlNUN0Y29uc29sZS5sb2coQ2hpbGRDb21wb25lbnQubmFtZSklM0IlNUNuJTIwJTIwJTdEJTVDbiU3RCUzQiU1Q25leHBvcnQlMjAlN0IlNUNuJTIwJTIwUGFyZW50Q29tcG9uZW50JTIwYXMlMjBkZWZhdWx0JTVDbiU3RCUzQiU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyY2hpbGQtc2NyaXB0LmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmxldCUyMENoaWxkQ29tcG9uZW50JTIwJTNEJTIwY2xhc3MlMjAlN0IlN0QlM0IlNUNuZXhwb3J0JTIwJTdCJTVDbiUyMCUyMENoaWxkQ29tcG9uZW50JTIwYXMlMjBkZWZhdWx0JTVDbiU3RCUzQiU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyY2hpbGQtY29tcG9uZW50LmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMF9zZmNfbWFpbiUyMGZyb20lMjAlNUMlMjIuJTJGY2hpbGQtc2NyaXB0LmpzJTVDJTIyJTNCJTVDbmV4cG9ydCUyMColMjBmcm9tJTIwJTVDJTIyLiUyRmNoaWxkLXNjcmlwdC5qcyU1QyUyMiUzQiU1Q25pbXBvcnQlMjBfX25vcm1hbGl6ZXIlMjBmcm9tJTIwJTVDJTIyLiUyRmNvbXBvbmVudC1ub3JtYWxpemVyLmpzJTVDJTIyJTNCJTVDbnZhciUyMF9fY29tcG9uZW50X18lMjAlM0QlMjAlMkYqJTIwJTQwX19QVVJFX18lMjAqJTJGJTIwX19ub3JtYWxpemVyKF9zZmNfbWFpbiklM0IlNUNuZXhwb3J0JTIwZGVmYXVsdCUyMF9fY29tcG9uZW50X18uZXhwb3J0cyUzQiU1Q24lMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIycGFyZW50LWNvbXBvbmVudC5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJpbXBvcnQlMjBfc2ZjX21haW4lMjBmcm9tJTIwJTVDJTIyLiUyRnBhcmVudC1zY3JpcHQuanMlNUMlMjIlM0IlNUNuZXhwb3J0JTIwKiUyMGZyb20lMjAlNUMlMjIuJTJGcGFyZW50LXNjcmlwdC5qcyU1QyUyMiUzQiU1Q25pbXBvcnQlMjBfX25vcm1hbGl6ZXIlMjBmcm9tJTIwJTVDJTIyLiUyRmNvbXBvbmVudC1ub3JtYWxpemVyLmpzJTVDJTIyJTNCJTVDbnZhciUyMF9fY29tcG9uZW50X18lMjAlM0QlMjAlMkYqJTIwJTQwX19QVVJFX18lMjAqJTJGJTIwX19ub3JtYWxpemVyKF9zZmNfbWFpbiklM0IlNUNuZXhwb3J0JTIwZGVmYXVsdCUyMF9fY29tcG9uZW50X18uZXhwb3J0cyUzQiU1Q24lMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyY29tcG9uZW50LW5vcm1hbGl6ZXIuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyJTJGJTJGJTIwU3RhbmQtaW4lMjBzaGltJTIwZm9yJTIwdGhlJTIwbGF5ZXIlMjBvZiUyMGluZGlyZWN0aW9uJTIwaW50cm9kdWNlZCUyMGJ5JTIwdnVlMidzJTIwY29tcG9uZW50LW5vcm1hbGl6ZXIuJTVDbmV4cG9ydCUyMGRlZmF1bHQlMjBmdW5jdGlvbihjb21wb25lbnQpJTIwJTdCJTVDbiU1Q3RyZXR1cm4lMjAlN0IlNUNuJTVDdCU1Q3RleHBvcnRzJTNBJTIwY29tcG9uZW50JTVDbiU1Q3QlN0QlNUNuJTdEJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

Expected Behaviour

The resulting output, when executed, prints:

ParentComponent
ChildComponent

Actual Behaviour

The resulting output, when executed, prints:

ParentComponent$1
ChildComponent$1

because the class's names are not preserved.

This repro is a very stripped down example of the output from the Vue component compiler. The real world scenario here is using vue-class-component, which uses the class name as the component name.

This scenario even follows this workaround, but it does not help.

Other related issues:

  • https://github.com/rollup/rollup/issues/343#issuecomment-1159022257 (cc @Sidnioulz)
  • https://github.com/nuxt/vite/issues/52 (cc @productdevbook @IlyaSemenov)

ascott18 avatar Sep 09 '22 17:09 ascott18

We might be able to address this by rewriting class Foo {} or let Foo = class {} as let Foo$1 = class Foo {} when we need to change the variable name, not sure if I am overlooking something here.

lukastaegert avatar Sep 10 '22 04:09 lukastaegert

I think that should work - I just tried making a very simple and naive transform plugin that does that (regardless of name or if it will be changed, since of course before chunking I can't know if it needs it or not) and it seems to work.

ascott18 avatar Sep 10 '22 04:09 ascott18

Then we should implement that in core, but not sure when I will get to it.

lukastaegert avatar Sep 10 '22 08:09 lukastaegert

Hey there! I'm stumbling upon this issue and I would be glad to help you fix this.

If you can give me a hint on where to look to fix this, it would be nice!

clementprevot avatar Oct 14 '22 07:10 clementprevot

First thing you can do is write a failing test. I would recommend looking into the test/function folder for what those look like. Basically they bundle some code and then run it, and then are usually green if no errors occur. When running the code, Node's assert module is available as a global assert variable which you can use to verify the class names.

The reproduction above may be slightly too involved for a test, but all you need is to cause a name conflict. The easiest way to achieve this is to have two modules in your bundle that use the same top-level variables.

Then to actually fix things, you need to adjust render functions of various AST nodes. For this solution https://github.com/rollup/rollup/issues/4637#issuecomment-1242623276 you would need to look into ClassDeclaration and VariableDeclarator. If you are unfamiliar with AST nodes, you can always use https://astexplorer.net to see what an AST would look like, make sure to set it to acorn.

lukastaegert avatar Oct 14 '22 07:10 lukastaegert

OK, I'll be honest. I tried to understand what exactly to do and where and sadly I failed. I have to recognize my weakness and let some more skilled people do this fix ๐Ÿ˜…

Thanks anyway for trying to explain to me ๐Ÿ‘๐Ÿป

I can't wait to see the PR to understand what I should have done

clementprevot avatar Oct 14 '22 14:10 clementprevot

No problem ๐Ÿ˜‰

lukastaegert avatar Oct 14 '22 15:10 lukastaegert

Here you go, fix at #4674

lukastaegert avatar Oct 14 '22 20:10 lukastaegert

Nice!

Thank you for two things: making this fix this quick and making me learn something ๐Ÿ˜ƒ

clementprevot avatar Oct 15 '22 15:10 clementprevot

This issue has been resolved via #4674 as part of [email protected]. You can test it via npm install rollup.

rollup-bot avatar Oct 16 '22 04:10 rollup-bot