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

Fix 2 lints in core/debounce.js #1021

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Fix 2 lints in core/debounce.js #1021

merged 1 commit into from
Jan 29, 2019

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Jan 29, 2019

src/core/debounce.js
  25:34  error  Unexpected 'this'                               babel/no-invalid-this
  36:18  error  Use the rest parameters instead of 'arguments'  prefer-rest-params

The use of this in this function is expected (required, even), so suppress that lint.

The use of arguments, however, isn't required, so I replaced it.

This function isn't like other debounce functions I've seen: it allows you to pass a set of default arguments at creation-time, and in the absence of arguments being passed at call-time you get the defaults. That sounds reasonable enough (although a more conventional way to do this would be to use bind()).

The weirder thing, though, is what happens if you supply defaults at creation-time and also pass arguments at call-time: in this case, the wrapped function ends up getting called with the defaults appended to the call-time args. I can't imagine a scenario in which this behavior would be non-surprising or useful, and the test suite doesn't even exercise this behavior, so perhaps this is accidental design (it does verify that default args get used in the absence of calltime args).

In any case, I am leaving the behavior intact and just making the changes to satisfy the linter. If we want, we can separately remove the weird concatenating behavior.

Usual test plan: npm run dev && npm run text && npm run start and play with demo.

Related: #990

```
src/core/debounce.js
  25:34  error  Unexpected 'this'                               babel/no-invalid-this
  36:18  error  Use the rest parameters instead of 'arguments'  prefer-rest-params
```

The use of `this` in this function is expected (required, even), so
suppress that lint.

The use of `arguments`, however, isn't required, so I replaced it.

This function isn't like other debounce functions I've seen: it allows
you to pass a set of default arguments at creation-time, and in the
absence of arguments being passed at call-time you get the defaults.
That sounds reasonable enough (although a more conventional way to do
this would be to use `bind()`).

The weirder thing, though, is what happens if you supply defaults at
creation-time *and* also pass arguments at call-time: in this case, the
wrapped function ends up getting called with the defaults *appended* to
the call-time args. I can't imagine a scenario in which this behavior
would be non-surprising or useful, and the test suite doesn't even
exercise this behavior, so perhaps this is accidental design (it *does*
verify that default args get used in the absence of calltime args).

In any case, I am leaving the behavior intact and just making the
changes to satisfy the linter. If we want, we can separately remove the
weird concatenating behavior.

Usual test plan: `npm run dev && npm run text && npm run start` and play
with demo.

Related: #990
@wincent
Copy link
Contributor Author

wincent commented Jan 29, 2019

Perhaps @jbalsas remembers why the debounce function does the weird concatenation thing. At least internally, there is nowhere in the project where we seem to be passing default args to debounce() (and just one place where we are passing a context).

@julien
Copy link
Contributor

julien commented Jan 29, 2019

Just started reviewing :)

:octocat: Sent from GH.

@julien julien merged commit 387ec57 into liferay:2.x-develop Jan 29, 2019
@wincent wincent deleted the lint/n+13 branch January 29, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants