Skip to content

fixes #1217: edge case with default watchOptions value #1218

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

Merged
merged 1 commit into from
Dec 7, 2017
Merged

fixes #1217: edge case with default watchOptions value #1218

merged 1 commit into from
Dec 7, 2017

Conversation

birdwell
Copy link
Contributor

@birdwell birdwell commented Dec 6, 2017

There is a problem when watchOptions is set to undefined in the options object. this.watchOptions is then set to undefined. We want an empty object so we can check poll property later.

What kind of change does this PR introduce?

bugfix
Did you add or update the examples/?
No.
Summary

Edge Case Issue

When Server(compiler, options) - options object looks like this:

{ 
  ...
  watchOptions: undefined,
  ...
 }

this.watchOptions will be set to undefined.

 this.watchOptions = options.watchOptions

These lines will give raise to the error specified in the issue request.

  const usePolling = this.watchOptions.poll ? true : undefined; // eslint-disable-line no-undefined
  const interval = typeof this.watchOptions.poll === 'number' ? this.watchOptions.poll : undefined; // eslint-disable-line no-undefined

Does this PR introduce a breaking change?

No.

Other information

There is a problem when watchOptions is set to undefined in the options object. this.watchOptions is then set to undefined. We want an empty object so we can check poll property later.
@jsf-clabot
Copy link

jsf-clabot commented Dec 6, 2017

CLA assistant check
All committers have signed the CLA.

@shellscape
Copy link
Contributor

@birdwell that was quick! checkout the comment by jsf-clabot on this PR. we'll need you to sign the CLA so we can merge your PR.

@codecov
Copy link

codecov bot commented Dec 7, 2017

Codecov Report

Merging #1218 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1218   +/-   ##
=======================================
  Coverage   76.31%   76.31%           
=======================================
  Files           5        5           
  Lines         477      477           
  Branches      153      154    +1     
=======================================
  Hits          364      364           
  Misses        113      113
Impacted Files Coverage Δ
lib/Server.js 82.89% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2719590...26b30b5. Read the comment docs.

@birdwell
Copy link
Contributor Author

birdwell commented Dec 7, 2017

Okay, I signed the CLA

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