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

Port over missing bindings from my deprecated repo #14

Merged
merged 16 commits into from Mar 2, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Feb 28, 2018

@zploskey
Copy link
Owner

zploskey left a comment

Looks good, just a couple of minor nits.


[@bs.send]
external _addScriptTag : (t, tagOptions) => Js.Promise.t(ElementHandle.t) =
"addScriptTag";

This comment has been minimized.

@zploskey

zploskey Feb 28, 2018

Owner

I'm wondering whether this should be an exposed function or not. If we expose addScriptTag only, then there would be no need for exposing makeTagOptions either. I'm inclined to call this external addScriptTagWithOptions or something like that and make it use @bs.send.pipe so there is a choice of using it for an API closer to upstream Puppeteer, or the shorter curryable option addScriptTag. If we choose not to expose this we will want to generate an interface file for this module.

This comment has been minimized.

@jihchi

jihchi Mar 1, 2018

Author Collaborator

I've changed to us @bs.send.pipe in commit e0aec13

);
);
test("defaultArgs()", () =>
Puppeteer.defaultArgs() |> expect |> toHaveLength(17)

This comment has been minimized.

@zploskey

zploskey Feb 28, 2018

Owner

This breaks if the number of default arguments change. Maybe we should just check that the length is greater than 0.

This comment has been minimized.

@jihchi

jihchi Feb 28, 2018

Author Collaborator

Thanks! Fixed in commit 4bf5e9f

@zploskey

This comment has been minimized.

Copy link
Owner

zploskey commented Feb 28, 2018

Let me know when you're happy with this. I would be fine with merging what is here.

@jihchi jihchi force-pushed the jihchi:port_over_missing_bindings branch from 48baea9 to 23775eb Mar 1, 2018

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Mar 1, 2018

@zploskey Thanks for review! done.


[@bs.send.pipe : t]
external evaluate :
(unit => Js.Promise.t(Js.Json.t), [@bs.splice] array({..})) =>

This comment has been minimized.

@zploskey

zploskey Mar 2, 2018

Owner

This will only ever work for functions taking no arguments, and the second parameter array can only ever be empty. Same for evaluateHandle. There are some ways around this we can experiment with, but I'd like to get this merged and we can iterate on it a bit.

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

Let me put TODO comment on it and improve it later.

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

Comment added in 067382b

@zploskey
Copy link
Owner

zploskey left a comment

The emulateMedia binding should be improved, but otherwise looks ready.

src/Page.re Outdated
| Screen
| Print;

let emulateMedia = (mediaType: option(mediaType), t) =>

This comment has been minimized.

@zploskey

zploskey Mar 2, 2018

Owner

This could all be boiled down to

[@bs.send.pipe : t]
external emulateMedia :
  (~mediaType: Js.nullable([@bs.string] [ | `screen | `print])=?) =>
  Js.Promise.t(unit) =
  "";

Then call with page |> Page.emulateMedia(`screen).

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

oops, the code will fail:

  370 ┆ testPromise("emulateMedia()", () =>
  371 ┆   Js.Promise.(
  372 ┆     page^ |> Page.emulateMedia(`print) |> then_(() => pass |> resolve
        )
  373 ┆   )
  374 ┆ );

  This has type:
    [> `print ]
  But somewhere wanted:
    BsPuppeteer.Page.t (defined as BsPuppeteer.FrameBase.t)

This comment has been minimized.

@zploskey

zploskey Mar 2, 2018

Owner

Oops yeah, we want to be able to call it like that. I guess it should not be optional.

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

emulateMedia binding is a little bit tricky here.

currently only allowed 'screen', 'print' and null. passing null disables media emulation.

I've tried this:

[@bs.send.pipe : t]
external emulateMedia :
  (~mediaType: Js.Nullable.t([@bs.string] [ | `screen | `print])=?) =>
  Js.Promise.t(unit) =
  "";

and with test case:

    1   testPromise("emulateMedia()", () =>
    2     Js.Promise.(
~   3       page^
+   4       |> Page.emulateMedia(Js.Nullable.return(`print))
+   5       |> then_(() => pass |> resolve)
    6     )
    7   );

with or without named argument mediaType have same result:

    Protocol error (Emulation.setEmulatedMedia): Invalid parameters media: string value expected

      268 |               }));
      269 |         Jest.testPromise(/* None */0, "emulateMedia()", (function () {
    > 270 |                 return page[0].emulateMedia(/* print */-930392019).then((function () {
      271 |                               return Promise.resolve(Jest.pass);
      272 |                             }));
      273 |               }));

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

I'm thinking about separate binding:

[@bs.send.pipe : t]
external emulateMedia :
  ([@bs.string] [ | `screen | `print]) => Js.Promise.t(unit) =
  "";

[@bs.send.pipe : t]
external emulateMediaNull : ([@bs.as {json|null|json}] _) => Js.Promise.t(unit) =
  "";

And add test case for each:

  testPromise("emulateMedia()", () =>
    Js.Promise.(
      page^ |> Page.emulateMedia(`print) |> then_(() => pass |> resolve)
    )
  );
  testPromise("emulateMediaNull()", () =>
    Js.Promise.(page^ |> Page.emulateMediaNull |> then_(() => pass |> resolve))
  );

(We need better naming for emulateMediaNull)

This comment has been minimized.

@zploskey

zploskey Mar 2, 2018

Owner

Yeah I was thinking similarly. What if we call the null one emulateMediaDisable, which is what it is doing. You need to set the = "emulateMedia" at the end of that external.

This comment has been minimized.

@jihchi

jihchi Mar 2, 2018

Author Collaborator

Fixed in commit 4d6b40b

);
testPromise("emulateMedia()", () =>
Js.Promise.(
page^ |> Page.emulateMedia(Some(Print)) |> then_(() => pass |> resolve)

This comment has been minimized.

@zploskey

zploskey Mar 2, 2018

Owner

I think this is nicer using polymorphic variants. See other comment.

@jihchi

This comment has been minimized.

Copy link
Collaborator Author

jihchi commented Mar 2, 2018

@zploskey done.

@zploskey

This comment has been minimized.

Copy link
Owner

zploskey commented Mar 2, 2018

Thanks for getting this done. Merging.

@zploskey zploskey merged commit 021fcbe into zploskey:master Mar 2, 2018

1 check passed

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

@jihchi jihchi deleted the jihchi:port_over_missing_bindings branch Mar 2, 2018

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