Skip to content
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

Use Promise in example for #evaluate_async #325

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ttilberg
Copy link
Contributor

Related to #234

I couldn't remember how to get a value back from evaluate_async and remembered I had created an example in a previous issue.

The existing example leverages arguments[0] and calls a simple object. Instead, I would like to see an actual, practical example using familiar things that people would commonly use this for.

Additionally, I think we should encourage usage of __f() as in the next example, instead of arguments[0], or rework the #evaluate_async template. As an example, if you call:

browser.evaluate_async '__f(arguments)', 5, "arg 1", 2, 3
# => ["arg 1", 2, 3, {}]

arguments[0] no longer refers to the fulfill callback. You end up receiving TypeError: arguments[0] is not a function (Ferrum::JavaScriptError).

Perhaps this highlights some tweaks that could be made to evaluate_async, but for now, I'd like to get this docs change up asap.

@route
Copy link
Member

route commented Jan 12, 2023

I don't like current API either, but I remember picking up the names for resolve and reject functions so that they don't collide with any other name (thus I added __), and now we are making them kinda explicit. Looking at:

browser.evaluate_async '__f(arguments)', 5, "arg 1", 2, 3

is so unreadable. I'm always confused when to put what, and use specs to remember how to use it. So maybe it's time to improve the API then if it's doable at all? :)

@alexeyr-ci
Copy link

Reminding about this PR because I just also was confused by arguments[0] in the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants