-
Notifications
You must be signed in to change notification settings - Fork 70
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
base: master
Are you sure you want to change the base?
Conversation
Any objections about this PR? |
@baudehlo Can you merge this PR? Or tell me - what is wrong with it? |
Why existing callbacks implementation can't be used for this https://github.com/baudehlo/node-phantom-simple/blob/2.2.4/bridge.js#L148? |
@puzrin Because onInitialized fires before onPageCreated, where all other callbacks set |
Vitaly runs this project now - I don't use the module so I'd rather someone
who does merges PRs.
…On Thu, Apr 13, 2017 at 6:27 PM, Evgeny Kruglov ***@***.***> wrote:
@puzrin <https://github.com/puzrin> Because onInitialized fires before
onPageCreated, where all other callbacks setup
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#144 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAobY3hEPP3nNWU2BolswJRqDOKh3nRkks5rvqFmgaJpZM4KKkYr>
.
|
@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. |
@puzrin You need to pass that function to |
Otherwise, there was no possibility to add real "onInitalized"