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

page event listener fix #420

Merged
merged 2 commits into from Jan 28, 2016
Merged

page event listener fix #420

merged 2 commits into from Jan 28, 2016

Conversation

antpaw
Copy link
Contributor

@antpaw antpaw commented Dec 29, 2015

untested

@matthewmueller
Copy link
Contributor

what does this fix?

@antpaw
Copy link
Contributor Author

antpaw commented Jan 2, 2016

it logs to the bash the page events like alert and error with DEBUG=nightmare:log

@rosshinkley
Copy link
Contributor

The change to page from page-error, page-log etc was an intentional one. Take a look at the default preload script, specifically at how page errors are handled and consequently transmitted to Nightmare. All of the messages are (to the best of my knowledge) still being logged to debug, but are logically grouped together using the same event.

@antpaw
Copy link
Contributor Author

antpaw commented Jan 5, 2016

it was intentional and i understand this change how ever only the child process does this correct (as you mentioned). https://github.com/segmentio/nightmare/blob/master/lib/runner.js#L90-L92

the parent process doesn't listen to page it listens to page-error and thats what die pr is fixing. https://github.com/segmentio/nightmare/blob/master/lib/nightmare.js#L111-L117

@rosshinkley
Copy link
Contributor

@antpaw Ah, I see. I must have misread the original diff. It looked like in passing it was adding the page-* events back. At any rate, I think you are correct. The parent process should listen to page. +1. :)

One nit: I think the page handler should handle variable arguments. Something like:

this.child.on('page', function(){                                                               
  log.apply(null, ['page'].concat(sliced(arguments)))                                           
}); 

... or if the page-* convention should be kept for debug, something like:

this.child.on('page', function(){                                                               
  log.apply(null, ['page-'+arguments[0]].concat(sliced(arguments,1)))                           
});

Re testing, the tests for ensuring page is handled are already there (alert, prompt, and confirm are good examples), but simply test that the events are emitted from the child and not submitted to debug. Testing debug output would probably require using a mock (like mockery).

@matthewmueller
Copy link
Contributor

I think the page handler should handle variable arguments

+1

then i think we're good to go :-)

matthewmueller added a commit that referenced this pull request Jan 28, 2016
@matthewmueller matthewmueller merged commit ec91c3e into segment-boneyard:master Jan 28, 2016
@matthewmueller
Copy link
Contributor

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants