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

Fix eval functions, allow them to take args #21

Merged
merged 6 commits into from Mar 8, 2018

Conversation

Projects
None yet
4 participants
@zploskey
Copy link
Owner

zploskey commented Mar 6, 2018

  • Makes eval functions on pages/frames able to take arguments to the evaluated function.
  • Creates versions of the eval functions that take a function to evaluate that does not return a promise.
  • Covers arities of evaluated functions up to 4 additional arguments.
  • Adds tests for some of these binding, but most of these bindings should behave similarly.

Fixes #6.

Fix eval functions, allow them to take args
Adds some additional functions and tests for most of them.
@zploskey

This comment has been minimized.

Copy link
Owner Author

zploskey commented Mar 6, 2018

@jihchi @Khady This API is very important to the usability of these bindings. I would really appreciate your feedback.

We may want to at least add a few more shortcut functions for calling eval functions that don't take arguments, and maybe make the ones that take extra arguments have withArgs appended to their names. I'm not sure to what extent currying could be used to eliminate the need to pass additional arguments to the eval functions separately.

Puppeteer API Docs: https://github.com/GoogleChrome/puppeteer/blob/v1.1.1/docs/api.md
Typescript definitions: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/puppeteer/index.d.ts

@zploskey
Copy link
Owner Author

zploskey left a comment

Looking at this with fresh eyes, this clearly needs some more work.

/* TODO: Add support [, ...args] */
[@bs.splice]
external selectOneEval :
(string, 'a => 'b, array(jsonOrHandle)) => Js.Promise.t(serializable) =

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

First argument of the eval function should be Dom.element, then accept any number of other arguments as passed in the array(jsonOrHandle). Might be good to make this the WithArgs version and drop the third argument for selectOneEval.

[@bs.send.pipe : t]
external selectAllEval : (string, unit => unit) => Js.Promise.t('a) = "$$eval";
external selectAllEval :
(string, Dom.element => 'a) => Js.Promise.t(serializable) =

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Actually don't think this will work as is. The first argument of the function should be a nodeList(Dom.element). Need to figure out what to use for a nodeList binding or if we need to write one.
https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll

This comment has been minimized.

@yawaramin

yawaramin Mar 8, 2018

You can use the type https://bucklescript.github.io/bucklescript/api/Dom.html#TYPEnodeList which was explicitly left as abstract so that different libraries could use the types interoperably.

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Perfect, we should definitely use that here. We may want to recommend a library for working with these types--maybe put a link to bs-webapi in the readme.


[@bs.send.pipe : t] [@bs.splice]
external selectAllEvalWithArgs :
(string, Dom.element => 'a, array(jsonOrHandle)) =>

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

As above, first arg to the eval function needs to be nodeList(Dom.element).

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

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Since I wound up splitting evaluate() into evaluateString and this function, we could maybe rethink whether we want to use Eval.Fn.t as the first argument, or if Eval.Fn.t needs to include string. We need to somehow indicate how many args the function takes, or require the user to curry all the args before passing the function to evaluate.

This comment has been minimized.

@yawaramin

yawaramin Mar 8, 2018

Having a separate function binding for the string argument seems like a good idea. It puts the user in an explicitly different context.

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Probably best to have bindings for functions of each arity for evaluate as well, so it is consistent. Then we can get rid of Eval.Fn and we only need Eval.Arg.

[@bs.send.pipe : t] [@bs.splice]
external evaluateHandle :
(unit => Js.Promise.t(JSHandle.t), array({..})) => Js.Promise.t(JSHandle.t) =
('a => Js.Promise.t(JSHandle.t), array(Eval.Arg.t)) =>

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

The first arg to evaluateHandle should be a function of any number of arguments (equal to the length of the array in the second arg to evaluateHandle, though I don't know if we can enforce that). Again, need to investigate whether we want/can require the user to curry the eval function with args so that we just take a function of type unit => Js.Promise.t(JSHandle.t).

This comment has been minimized.

@yawaramin

yawaramin Mar 8, 2018

It looks like, to correctly compile to uncurried callbacks for the pageFunction, you'll need to use [@bs] or [@bs.uncurry]. This in turn means that you'll need to provide multiple external bindings by arity. E.g.,

[@bs.send.pipe: t] [@bs.splice] external evaluateHandle1: (
  [@bs.uncurry] Eval.Arg.t => Js.Promise.t(JSHandle.t),
  Eval.Arg.t) =>
  Js.Promise.t(JSHandle.t) =
  "evaluateHandle";

[@bs.send.pipe: t] [@bs.splice] external evaluateHandle2: (
  [@bs.uncurry] (Eval.Arg.t, Eval.Arg.t) => Js.Promise.t(JSHandle.t),
  Eval.Arg.t,
  Eval.Arg.t) =>
  Js.Promise.t(JSHandle.t) =
  "evaluateHandle";

And so on.

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Ah this is much better. Explicitly enumerating the the args means we constrain their number and we can get rid of [@bs.splice] since they won't need to pass an array.

src/Eval.re Outdated
external fn1 : ('a => 'b) => t = "%identity";
external fn2 : (('a, 'b) => 'c) => t = "%identity";
external fn3 : (('a, 'b, 'c) => 'd) => t = "%identity";
external fn4 : (('a, 'b, 'c, 'd) => 'd) => t = "%identity";

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Is there a better way to do this?

This comment has been minimized.

@Khady

Khady Mar 8, 2018

external fn4 : (('a, 'b, 'c, 'd) => 'd) => t = "%identity";`

Should be

external fn4 : (('a, 'b, 'c, 'd) => 'e) => t = "%identity";
@zploskey
Copy link
Owner Author

zploskey left a comment

.

src/Eval.re Outdated
external fn1 : ('a => 'b) => t = "%identity";
external fn2 : (('a, 'b) => 'c) => t = "%identity";
external fn3 : (('a, 'b, 'c) => 'd) => t = "%identity";
external fn4 : (('a, 'b, 'c, 'd) => 'd) => t = "%identity";

This comment has been minimized.

@Khady

Khady Mar 8, 2018

external fn4 : (('a, 'b, 'c, 'd) => 'd) => t = "%identity";`

Should be

external fn4 : (('a, 'b, 'c, 'd) => 'e) => t = "%identity";
@zploskey

This comment has been minimized.

Copy link
Owner Author

zploskey commented Mar 8, 2018

Using the Eval.Args.t proved unwieldy when it came to actually writing functions to evaluate in page context in Reason. I rewrote the bindings to be generic, but as long as the function to be evaluated is typed then the return types are known, and this lets us write nice clean functions to evaluate. Args can be passed as their normal values. Seems to work well.

This also let us get rid of serializable, as the return type is derived from the type of the evaluated function. Values returned from the eval functions should not generally need to be decoded from json. Overall I think this is pretty clean usage for the user, so bloating the bindings a bit by covering some of the different cases with various externals is worth it.

I'm pretty happy with this. Let me know if you have any concerns before I merge this.

*/
[@bs.send.pipe : t]
external selectOneEvalPromise :
(string, Dom.element => Js.Promise.t('r)) => Js.Promise.t('r) =

This comment has been minimized.

@jihchi

jihchi Mar 8, 2018

Collaborator

Just curious, why need Promise version (Dom.element => Js.Promise.t('r)) here? Would Dom.element => 'r not be enough?

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

Then the return type of the external is potentially Js.Promise.t(Js.Promise.t('r)) or 'r which is not a promise, which are both incorrect. Javascript promises can't be nested like that and are flattened to one under the hood. The puppeteer docs also state it will always only return one promise. We need the return type from inside the promise returned from the evaled function, otherwise we can have a return type that is not a promise at all if they pass a function that does not return a promise. I know it seems odd but I'm not sure that there are any other straightforward ways of dealing with this.

This comment has been minimized.

@zploskey

zploskey Mar 8, 2018

Author Owner

If you need to eval a function that does not return a promise then use selectOneEval instead of this function. The types will keep you honest as long as there is a known return type (i.e. not 'a or the like). On a side note, we're going to want to write and generate API docs at some point since we are deviating a bit from the upstream API, and it's nice to see it in the editor. I've started to write some here.

@zploskey

This comment has been minimized.

Copy link
Owner Author

zploskey commented Mar 8, 2018

Using [@bs.uncurry] doesn't seem to be necessary the way this is now written. It generates the same code either way and passing curried functions just works.

@zploskey zploskey force-pushed the betterEval branch from 802b166 to c001d64 Mar 8, 2018

@zploskey zploskey merged commit b2a0781 into master Mar 8, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Owner Author

zploskey commented Mar 8, 2018

If there are issues with these changes let's just do some bug fixes.

@zploskey zploskey deleted the betterEval branch Mar 8, 2018

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