node-phantom-simple icon indicating copy to clipboard operation
node-phantom-simple copied to clipboard

Added onInitalized behaviour and fixed unmet dev dependency

Open nordluf opened this issue 8 years ago • 7 comments

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

nordluf avatar Sep 29 '16 22:09 nordluf

Any objections about this PR?

nordluf avatar Dec 05 '16 21:12 nordluf

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

nordluf avatar Apr 13 '17 21:04 nordluf

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 avatar Apr 13 '17 21:04 puzrin

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

nordluf avatar Apr 13 '17 22:04 nordluf

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 [email protected] 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 https://github.com/baudehlo/node-phantom-simple/pull/144#issuecomment-294037816, or mute the thread https://github.com/notifications/unsubscribe-auth/AAobY3hEPP3nNWU2BolswJRqDOKh3nRkks5rvqFmgaJpZM4KKkYr .

baudehlo avatar Apr 13 '17 22:04 baudehlo

@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 avatar Apr 13 '17 22:04 puzrin

@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.

nordluf avatar Apr 13 '17 23:04 nordluf