Skip to content

do not warn if polling interval is undefined#161

Merged
a-b-r-o-w-n merged 2 commits intomicrosoft:masterfrom
compulim:fix-remove-undefined-warning
Feb 25, 2019
Merged

do not warn if polling interval is undefined#161
a-b-r-o-w-n merged 2 commits intomicrosoft:masterfrom
compulim:fix-remove-undefined-warning

Conversation

@compulim
Copy link
Copy Markdown
Collaborator

@compulim compulim commented Feb 17, 2019

Fix #160.

Background

After our recent pull #157, a warning message will pop up if pollingActivity does not fall into our acceptable range and being overridden.

The warning message should not show up if the pollingActivity is undefined. This is because we should assume if the user provide undefined for an options, they prefer default value. Thus, the warning is redundant in this case.

This PR is to suppress the warning message for undefined case.

Changelog

Fixed

Comment thread src/directLine.ts

if (parsedPollingInterval < POLLING_INTERVAL_LOWER_BOUND) {
console.warn(`DirectLineJS: provided pollingInterval (${ options.pollingInterval }) is under lower bound (200ms), using default of 1000ms`);
if (typeof options.pollingInterval !== 'undefined') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why typeof, and not just options.pollingInterval === undefined?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a JS idiom used because in some cases the identifier being check may not have been initialized yet. That's not the case here, but it doesn't hurt using typeof.

@a-b-r-o-w-n a-b-r-o-w-n changed the title Remove warning if undefined Remove warning if polling interval is undefined Feb 25, 2019
@a-b-r-o-w-n a-b-r-o-w-n changed the title Remove warning if polling interval is undefined do not warn if polling interval is undefined Feb 25, 2019
@a-b-r-o-w-n a-b-r-o-w-n merged commit f876daa into microsoft:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants