Add non-required debounceOptions prop object and forward it to lodash

[ghost]

This PR is for #93 and it works very well for my own use case.

I tried my best to make changes with minimal impact. Tests were done manually with my own project, no test code or example was written.

Perhaps a separate property per option would be preferred, however I chose passing a single object so that there's only one extra strict comparison to make in componentWillReceiveProps and also because it's less work to pass the options directly to debounce(), which already takes care of the case where debounceOptions is undefined.

Thanks! 🎉

[ghost]

Whoops, I forgot to lint! I'll make a new PR squashed into one commit if you want.

Thanks again!

[nkbt]

comparing {} !== {} would trigger updates on each render. Unfortunately for objects we need to use shallow object compare instead.

[ghost]

I am not sure when {} !== {} is actually going to happen. The default is undefined. If someone sets the property, the object will stay the same until they change it.

[nkbt]

Sorry for a nit, but I usually do named arguments if there are more then one. Like ({debounceTimeout, debounceOptions})

[ghost]

These are named arguments: (debounceTimeout, debounceOptions) => and this is destructuring named properties from the first argument: ({debounceTimeout, debounce}) => . If you change it to the latter, then every call to createNotifier will have to create a new object (unless of course you have a single object instance that you keep around to pass to it) e.g. createNotifier({debounceTimeout: theTimeout, debounceOptions: theOptions}) (which of course could be shortened if the variables are named the same as the properties).

[nkbt]

Yeah, right. Destructuring in JS is the only way of doing "named arguments" thing, which just does not exist in JS natively. Gives more clarity to the code that calls this sort of function.

[nkbt]

Thanks! No need to squash, I actually like full history even if it is ridiculously big. git rebase and git push --force is my usual daily git routine...

[ghost]

Thanks for looking at this. If you want me to continue making changes just let me know. Otherwise I will leave things to you. I don't want to step on your toes if you have concerns which differ from mine.

[nkbt]

It's all good, the only concern is object comparison, and I'll happily merge the change!

[ghost]

OK, I get what you're saying - you want to protect people who don't replace the entire debounceOptions object when they change a field of debounceOptions, correct?

[ghost]

I got it, I'll make that change and include it here - thanks.

[nkbt]

Not really for protection. It is more for code readability.

with "positional" args

this. createNotifier(first, second)

// ...

createNotifier = (debounceTimeout, debounceOptions) => {
  // do things, all works
}

with "named" (via destructuring) args:

this. createNotifier({debounceTimeout: first, debounceOptions: second})

// ...

createNotifier = ({debounceTimeout, debounceOptions}) => {
  // do things, all works
}

Second example gives a little more idea of what those arguments to called function are by explicitly stating their names.

PS: not much to do with this project, I just try to follow this convention in all the code I work on, so it is easier for other people to pick what is going on (myself in 1 year time falls into "other people" category pretty well too)

[ghost]

So, I was wondering how you want to do the shallow object comparison? My vote is either for shallowequal or separate properties for each option and let PureComponent do the work as explained below.

There is react-addons-shallow-compare however the repo states that it is no longer maintained and it's legacy and to use PureComponent instead, which you are already doing.

Since you're using PureComponent, perhaps we should consider doing a separate prop for each field of debounceOptions instead? That would be three new props, one for each of leading, trailing and maxWait.

Lodash doesn't have a shallow object compare function, the author recommends to compose your own function with _.isEqualWith (which does a deep comparison by default). This one seems to fill that void and is popular enough IMO - shallowequal.

TBH, I didn't even think about this the first time because I swap the whole object every time (or use immutable.js which basically does that for me).

[nkbt]

Any extra runtime dep adds to the bundle size =(
Since all lodash options are known, we could just manually check them one by one.

[ghost]

Regarding the named arguments via destructuring...if I make that change, then how do you want to change compentWillMount()?

I would be tempted to do this:

componentWillMount() {
    this.createNotifier(this.props);
  }

Instead of doing the change I originally made:

componentWillMount() {
    const {
      debounceTimeout,
      debounceOptions
    } = this.props;
    this.createNotifier(debounceTimeout, debounceOptions);
  }

which kinda defeats the point of using named arguments to begin with.

Would you rather see this?

componentWillMount() {
    const {
      debounceTimeout,
      debounceOptions
    } = this.props;
    this.createNotifier({debounceTimeout, debounceOptions});
  }

Thanks!

[ghost]

OK, I'll roll back the last 2 commits and do a manual check.

[nkbt]

I think the last example would be the most explicit and clear.

[ghost]

So, I am going to give up on this pull request because I don't like any of these changes. None of this is really my concern and this library is so simple that I could just pull it into my project.

I don't like doing these extra comparisons because they get too awkward when you take into account that the object can be null or undefined or they could be be a separate instance of a blank object{}. I'd never write this code for myself and I'd probably just use a library that one of my other dependencies is already using (shallowequal or an equivalent is probably already in my node_modules...)

I also don't like going out of the way to use destructuring for named arguments because it causes new unnecessary objects to be created for each and every call to a function for very little benefit IMO. I don't do this in my own code. My solution to the lack of named arguments is to be consistent with variable names and use the same name everywhere.

Sorry for wasting your time! Here was my simple attempt at writing a naive, pre-known object key comparison:

function comp(debounceTimeout, debounceOptions, debounceTimeoutCurrent, debounceOptionsCurrent) {
    if (
      debounceTimeout !== debounceTimeoutCurrent || !(
        // TODO: Make sure both objects aren't undefined/null here.
        // Otherwise, do extra comparisons which makes this whole block ugly and awkward.
        // Personally, I'm passing immutable props, so the simple comparison will work 
        // and my project won't use this. If I were making a library for others to use though, 
        // I'd probably just make a prop for each and let PureComponent do the work TBH.
        debounceOptions.leading === debounceOptionsCurrent.leading &&
        debounceOptions.trailing === debounceOptionsCurrent.trailing &&
        debounceOptions.maxWait === debounceOptionsCurrent.maxWait
      )
    ) { return 'changed'; } else { return 'equals'; }
}

[ghost]

Also, I was writing and testing that comp function in about:blank in devtools, so that's why it's structured the way it is...

[nkbt]

No worries, thanks anyway!