Skip to content
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

New rollup version results in different generated output and drops some if/else cases #4504

Closed
digaus opened this issue May 19, 2022 · 11 comments · Fixed by #4510
Closed

New rollup version results in different generated output and drops some if/else cases #4504

digaus opened this issue May 19, 2022 · 11 comments · Fixed by #4510

Comments

@digaus
Copy link

digaus commented May 19, 2022

Rollup Version

2.74.0

Operating System (or Browser)

MacOS, Linux, Windows

Node Version (if applicable)

12.x.x

Link To Reproduction

https://github.com/digaus/rollup-demo

Expected Behaviour

Since 2.74.0 angular lib compilation now produces different and wrong results.

This is a major issue for us and might break other libs and applications because it is not noticed while compiling. It completely drops use cases...

This happens when bundling to FESM2015. In ESM2015 code still looks fine...

Adding "rollup": "2.73.0" to the package.json fixes this for now...

See https://github.com/digaus/rollup-demo for reproduction

Actual Behaviour

Do not drop any code :)

@lukastaegert
Copy link
Member

Interestingly, vanilla Rollup does not seem to remove anything here, guess I will need to dig deeper

@costingeana
Copy link

costingeana commented May 19, 2022

Hello, guys,
I observed this behavior in the morning, but I thought I am doing something wrong. Please compare the following code snippets:

  • code is removed (it should not): link
  • code is not removed: link
  • code is not removed: link

I hope they help you to identify the issue(s).

@digaus
Copy link
Author

digaus commented May 19, 2022

Hello, guys, I observed this behavior in the morning, but I thought I am doing something wrong. Please compare the following code snippets:

  • code is removed (it should not): link
  • code is not removed: link
  • code is not removed: link

I hope they help you to identify the issue(s).

Yes same here. Thought I was too stupid 🤷‍♂️ Refactored code and had the issue from the other issue here. That's how I identified rollup as the cause.

@lukastaegert
Copy link
Member

Hello, guys,
I observed this behavior in the morning, but I thought I am doing something wrong. Please compare the following code snippets:
code is removed (it should not): link
code is not removed: link
code is not removed: link
I hope they help you to identify the issue(s).

All of them look correct to me:

  • In the first example, f2 does not have a side effect and the return value is not used -> remove
  • In the second example, the return is used -> we need to keep it
  • In the third example, f2 has a side effect in calling a global function -> keep it

@digaus
Copy link
Author

digaus commented May 19, 2022

Hello, guys,
I observed this behavior in the morning, but I thought I am doing something wrong. Please compare the following code snippets:
code is removed (it should not): link
code is not removed: link
code is not removed: link
I hope they help you to identify the issue(s).

All of them look correct to me:

  • In the first example, f2 does not have a side effect and the return value is not used -> remove
  • In the second example, the return is used -> we need to keep it
  • In the third example, f2 has a side effect in calling a global function -> keep it

What about my use case? 🙈

@lukastaegert
Copy link
Member

I will need more time for the original issue. The repo will help, but I have some family things coming up. Therefore I reverted the main feature from 2.74.0 for now in a 2.74.1 release so that I can take some time to understand the issue properly. Hope 2.74.1 fixes your issue for now? But please keep this one open as I want to roll back the revert soon once I got things figured out.

@costingeana
Copy link

costingeana commented May 20, 2022

Hello again,
Indeed, my first example wasn't good for highlighting the problem I encountered. I apologize.
I attached below 2 more code snippets that are closer to the code where I observed the problem. The code is a bit messy, but I tried to preserve the context.

@digaus
Copy link
Author

digaus commented May 20, 2022

@lukastaegert
2.74.1 seems to work for now, thank you for the fast release 👍

@costingeana
Copy link

I can also confirm that 2.74.1 is ok. Thank you.

@lukastaegert
Copy link
Member

I created a new PR #4510 that reimplements parameter tree-shaking in a more conservative way. Could you verify that this PR works for you via npm install rollup/rollup#parameter-defaults?

@costingeana
Copy link

Hello and sorry for the delay,
I confirm that the PR works for me. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants