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

Parameter's default value is removed when it's not supposed to #4516

Closed
jindrahm opened this issue May 30, 2022 · 7 comments · Fixed by #4518 or amalitsky/new-building-story#4
Closed
Assignees

Comments

@jindrahm
Copy link

jindrahm commented May 30, 2022

Rollup Version

2.75.0+

Operating System (or Browser)

macOS Monterey

Node Version (if applicable)

16.14.0

Link To Reproduction

https://replit.com/@jindrahm/ParamDefaultBug?v=1

Expected Behaviour

The following code should remain basically untouched after bundling by rollup.

const systemService = {restart};

function restart(options = {}) {
  if (options.countdown) {
    console.log('shows restart dialog with a countdown');
  } else {
    console.log('restarts immediately');
  }
}

systemService.restart();

Actual Behaviour

But it generates this code which throws an error because rollup removed the options param default value so it cannot access the countdown property of undefined.

const systemService = {restart};

function restart(options) {
    if (options.countdown) {
    console.log('shows restart dialog with a countdown');
  } else {
    console.log('restarts immediately');
  }
}

systemService.restart();
@ggmartins091
Copy link

Up.
I'm having the same problem here, using some methods from MUI packages where there are default parameters with empty objects, they are resolved to undefined.

Version 2.75.3 does not fix it.

@lukastaegert
Copy link
Member

Sorry for this. Fix at #4518

@inukshuk
Copy link

I think this is not completely fixed yet. I'm trying to build a minimal test case, but meanwhile I have a module including a function with the signature: export function f(a, b = {}, c = new Set()) which is imported into two bundles. In one of the bundles the parameters end up defined as (a, b, c) in the other as (a, b, c = new Set()) -- if I use the configuration of either of the two bundles but change to bundle only the module exporting the function, both default parameters stay intact. So it looks to me like the way the functions are used have an effect on which parameter defaults survive bundling.

@lukastaegert
Copy link
Member

So it looks to me like the way the functions are used have an effect on which parameter defaults survive bundling.

Well, that is the whole point of default parameter tree-shaking. The question is just if it is missing in a situation where it is definitely making a difference.

@ggmartins091
Copy link

ggmartins091 commented May 31, 2022

So it looks to me like the way the functions are used have an effect on which parameter defaults survive bundling.

Well, that is the whole point of default parameter tree-shaking. The question is just if it is missing in a situation where it is definitely making a difference.

I've just tested with version 2.75.4 and this issue is still reproducible: vitejs/vite#8395 (comment)

Its is basically something like this:

function foo() {
	const bar = (options = {}) => {
		console.log(options);
	}
	return bar;
}

foo()(); // console.logs "undefined"

@inukshuk
Copy link

Well, that is the whole point of default parameter tree-shaking. The question is just if it is missing in a situation where it is definitely making a difference.

Yes, sorry, I just ran into the issue before even reading up on the feature itself! In my case I could narrow it down to the use of try/catch blocks. For example, given a function defined as:

export function uniq(array, into = [], memo = new Set()) { /* ... */ }
import { uniq } from './util'

export function x() {
  try {
    return uniq([])

  } catch {
    return []
  }
}

Yields a function uniq(array, into, memo) without default parameters.

While a usage like:

export function y() {
  return uniq([])
}

Includes all of them. In my own code base there must be a third case which I haven't been able to isolate yet, where only the default value of the second parameter is dropped.

@lukastaegert
Copy link
Member

Oh darn. First, thanks so much for spotting these. It seems this feature that I initially thought was "not too complicated" is a pesky can of worms that is more brittle than any optimization I created before. I think at this point, I will keep the test cases (and create more regression tests for the things mentioned above), but roll back the feature until I am feeling adventurous again.

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