gruntjs / grunt-lib-phantomjs

Grunt and PhantomJS, sitting in a tree.
http://gruntjs.com/
MIT License
93 stars 63 forks source link

options.killTimeout has no default value. #80

Closed benhutchins closed 8 years ago

benhutchins commented 9 years ago

I'm fairly certain this line is incorrect:

if (typeof options.killTimeout !== 'number') { options.timeout = 5000; }

It checks to see if options.killTimeout is not a number (most likely it will be undefined), and then sets the options.timeout variable. This means that for the setTimeout will still always have an undefined timeout, this results in the setTimeout being executed immediately. Rendering both the "immediate" and the "setTimeout" features here useless.

The only way to actually use the killTimeout option is to explicitly set the option currently.

Arkni commented 8 years ago

Good catch. Sorry for ignoring the PR all this time.

@vladikoff I think the default killTimeout value should be replaced with a smallest value (500ms or 1000ms for example) or the process will looks like it hangs for some time (5s) then continue its work.

benhutchins commented 8 years ago

Shy of a year, so you're all still better than most open source maintainers :+1:

Arkni commented 8 years ago

Shy of a year, so you're all still better than most open source maintainers :+1:

haha, you can count me as a new member :-) And this PR was on my TODO for some time. I just found the time to check it.

vladikoff commented 8 years ago

@Arkni sure! feel free to make the change

Arkni commented 8 years ago

Merged. Thanks :)

Arkni commented 8 years ago

@Arkni sure! feel free to make the change

I'll update it in another commit