allow set debounceTime #260
Conversation
src/index.js
Outdated
| @@ -1580,10 +1583,11 @@ module.exports = { | |||
| this.settings.routes.forEach(route => this.addRoute(route)); | |||
|
|
|||
| // Regenerate all auto aliases routes | |||
| const debounceTime = (debounceTime => (!isNaN(debounceTime) && debounceTime >= 0) ? debounceTime : 500)(parseInt(this.settings.debounceTime)); | |||
There was a problem hiding this comment.
I feel it's a little too complicated. Just use a simply:
const debounceTime = debounceTime > 0 ? Number(this.settings.debounceTime) : 500
There was a problem hiding this comment.
I mean
- if
this.settings.debounceTimeis not set,parseIntwill be NaN - if
this.settings.debounceTimeis set, but
- set to a wrong value likes 'a',
parseIntwill be NaN debounceTimecan not less than 0 too
- If pass all conditions, take return value from
parseInt, else take default500
This line is short for
function parseDebounceTime(settingValue) {
const tmp = parseInt(settingValue);
if (!isNaN(tmp) && tmp > 0) return tmp;
else return 500
}
const debounceTime = parseDebounceTime(this.settings.debounceTime)There was a problem hiding this comment.
I understand, but the logic is too complex. The option will be set by developers, and I don't want to protect the developers from themselves. If somebody set "a" for a debounceTime option....
There was a problem hiding this comment.
@icebob backlog idea: maybe the fastest-validator would help here to check the configuration parameters?
src/index.js
Outdated
| @@ -1580,10 +1583,11 @@ module.exports = { | |||
| this.settings.routes.forEach(route => this.addRoute(route)); | |||
|
|
|||
| // Regenerate all auto aliases routes | |||
| const debounceTime = (debounceTime => (!isNaN(debounceTime) && debounceTime >= 0) ? debounceTime : 500)(parseInt(this.settings.debounceTime)); | |||
There was a problem hiding this comment.
I understand, but the logic is too complex. The option will be set by developers, and I don't want to protect the developers from themselves. If somebody set "a" for a debounceTime option....
src/index.js
Outdated
| @@ -1580,10 +1583,11 @@ module.exports = { | |||
| this.settings.routes.forEach(route => this.addRoute(route)); | |||
|
|
|||
| // Regenerate all auto aliases routes | |||
| const debounceTime = this.settings.debounceTime >= 0 ? parseInt(this.settings.debounceTime) : 500; | |||
There was a problem hiding this comment.
this.settings.debounceTime >= 0 is not correct. Just this.settings.debounceTime > 0
|
Hi debounceTime === 0 is same with setTimeout 0 and will be executed on
process next tick
…On Wed, Jul 21, 2021 at 14:30 Icebob ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/index.js
<#260 (comment)>
:
> @@ -1580,10 +1583,11 @@ module.exports = {
this.settings.routes.forEach(route => this.addRoute(route));
// Regenerate all auto aliases routes
+ const debounceTime = this.settings.debounceTime >= 0 ? parseInt(this.settings.debounceTime) : 500;
this.settings.debounceTime >= 0 is not correct. Just this.settings.debounceTime
> 0
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#260 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHX6SL7YJMKJ5KWAZ2OQZNTTYZZQTANCNFSM5AHYQNUA>
.
|
|
Yeah, just |
|
Oh, new knowledge lol
…On Wed, Jul 21, 2021 at 14:36 Icebob ***@***.***> wrote:
Yeah, just null >= 0 is true, so if somebody set it to null it should be
used the default 500 value.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#260 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHX6SLY3MYPEIPDFP7F5RW3TYZ2IHANCNFSM5AHYQNUA>
.
|
allow to change default
500msat line 1583, that will be called at line 1529