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

Add bindings for CDPSession and Target #43

Merged
merged 8 commits into from Apr 10, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Apr 1, 2018

Add bindings:

  Page
    ✓ target()
  Target
    ✓ page (1ms)
    ✓ type
    ✓ url (1ms)
    ✓ createCDPSession (1ms)
  CDPSession
    ✓ detach (190ms)
    ✓ send (49ms)

Fix #39 and #29

Archie Lee

@jihchi jihchi changed the title Add bindings for CDPSession, Add bindings for CDPSession Apr 2, 2018

Archie Lee added some commits Apr 5, 2018

Archie Lee
Archie Lee
Archie Lee
Archie Lee
Archie Lee

@jihchi jihchi requested a review from zploskey Apr 6, 2018

@jihchi jihchi changed the title Add bindings for CDPSession Add bindings for CDPSession and Target Apr 6, 2018

@zploskey
Copy link
Owner

zploskey left a comment

Most of this is just fine, but I'd like to address the EventEmitter business. I would like to take a different approach than what you've done here. The on function needs to be able to handle various functions, some with parameters and return values and some not. If we do wind up trying to do this in this repo, I would like it to be its own PR or series of PRs because it is not trivial. I filed an issue for this in #45. Would you mind removing the event stuff for now? Thanks!

Archie Lee
@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Apr 8, 2018

No problem! Fixed in commit 08d613d

[@bs.send] external detach : t => Js.Promise.t(unit) = "";

[@bs.send]
external send : (t, string, Js.t({..})) => Js.Promise.t(Js.t({..})) = "";

This comment has been minimized.

@zploskey

zploskey Apr 8, 2018

Owner

The third argument params is optional.

This comment has been minimized.

@jihchi

jihchi Apr 8, 2018

Author Collaborator

Fixed in commit a980bc6

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Apr 10, 2018

@zploskey PR is done, could you please review it?

@zploskey zploskey merged commit d340128 into zploskey:master Apr 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zploskey zploskey referenced this pull request Apr 10, 2018

Closed

Bindings for Target #29

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