-
Notifications
You must be signed in to change notification settings - Fork 2
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
Change runJS parameter passing and return value #535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These code changes look good and align with what we talked about in the dev team meeting.
+1 from me, but I'd appreciate it if you get a +1 from @gerardo-rodriguez as well, since some of this stuff is a little over my head, and he's more familiar with the scenario that led to this change.
expect(result.default.toString()).toEqual('JSHandle:hi'); | ||
|
||
expect(await result.a.jsonValue()).toStrictEqual(25); | ||
expect(await result.b.jsonValue()).toStrictEqual({}); // function is not JSON-serializable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work, @calebeby, yay! 🎉
And the documentation is excellent! Thank you so much. 💯
Closes #522
There are only like ~15 lines of code changes here! I added a lot of comments, tests, and documentation.
Recommend reviewing with whitespace changes hidden.
Overview of the changes in the changesets: