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

Added onInitalized behaviour and fixed unmet dev dependency #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added onInitalized behaviour and fixed unmet dev dependency #144

wants to merge 3 commits into from

Conversation

nordluf
Copy link

@nordluf nordluf commented Sep 29, 2016

Otherwise, there was no possibility to add real "onInitalized"

@nordluf
Copy link
Author

nordluf commented Dec 5, 2016

Any objections about this PR?

@nordluf
Copy link
Author

nordluf commented Apr 13, 2017

@baudehlo Can you merge this PR? Or tell me - what is wrong with it?

@puzrin
Copy link
Contributor

puzrin commented Apr 13, 2017

Why existing callbacks implementation can't be used for this https://github.com/baudehlo/node-phantom-simple/blob/2.2.4/bridge.js#L148?

@nordluf
Copy link
Author

nordluf commented Apr 13, 2017

@puzrin Because onInitialized fires before onPageCreated, where all other callbacks set

@baudehlo
Copy link
Owner

baudehlo commented Apr 13, 2017 via email

@puzrin
Copy link
Contributor

puzrin commented Apr 13, 2017

@baudehlo i don't run it too, because switched to electron.

I could merge something small and clear, but this PR change signatures too much, and adding new hash to param to pass single function looks suspicious. Reviewing this requires deep diving into existing logic and i can't do that now.

@nordluf
Copy link
Author

nordluf commented Apr 13, 2017

@puzrin You need to pass that function to createPage method, because only there you can setup onInitialized, right after page object created, before page load process starts. And I tried to do it in a way to keep backward compatibility.

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