feat(ObjectHandles): Implement Object Handles #943

aslushnikov · 2017-10-03T01:38:00Z

This patch:

introduces ExecutionContext class that incapsulates javascript
execution context. Examples of execution contexts are workers and
frames
introduces ObjectHandle that holds a references to the javascript
object in ExecutionContext
inherits ElementHandle from ObjectHandle

Fixes #382.

a1ph · 2017-10-05T20:58:07Z

docs/api.md

+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.
+
+```js
+const aHandle = await page.object(() => {


nit: () => Promise.resolve(window)

pavelfeldman · 2017-10-04T23:20:48Z

docs/api.md

@@ -109,21 +110,44 @@
    + [frame.$eval(selector, pageFunction[, ...args])](#frameevalselector-pagefunction-args)
    + [frame.addScriptTag(url)](#frameaddscripttagurl)
    + [frame.childFrames()](#framechildframes)
+    + [frame.context()](#framecontext)


executionContext

pavelfeldman · 2017-10-04T23:21:16Z

docs/api.md

@@ -55,6 +55,7 @@
    + [page.keyboard](#pagekeyboard)
    + [page.mainFrame()](#pagemainframe)
    + [page.mouse](#pagemouse)
+    + [page.object(pageFunction, ...args)](#pageobjectpagefunction-args)


evaluateObject

pavelfeldman · 2017-10-05T20:42:16Z

docs/api.md

    + [frame.evaluate(pageFunction, ...args)](#frameevaluatepagefunction-args)
    + [frame.injectFile(filePath)](#frameinjectfilefilepath)
    + [frame.isDetached()](#frameisdetached)
    + [frame.name()](#framename)
+    + [frame.object(pageFunction, ...args)](#frameobjectpagefunction-args)


pavelfeldman · 2017-10-05T20:43:26Z

docs/api.md

+  * [class: ObjectHandle](#class-objecthandle)
+    + [objectHandle.asArray()](#objecthandleasarray)
+    + [objectHandle.asElement()](#objecthandleaselement)
+    + [objectHandle.asJSON()](#objecthandleasjson)


pavelfeldman · 2017-10-05T20:43:45Z

docs/api.md

+    + [objectHandle.asArray()](#objecthandleasarray)
+    + [objectHandle.asElement()](#objecthandleaselement)
+    + [objectHandle.asJSON()](#objecthandleasjson)
+    + [objectHandle.asObject()](#objecthandleasobject)


getProperties(own) ? Returns Map.

pavelfeldman · 2017-10-05T20:49:05Z

docs/api.md

  * [class: ElementHandle](#class-elementhandle)
+    + [elementHandle.asArray()](#elementhandleasarray)


Return null for everything not array. Don't assign non-indexed property.

pavelfeldman · 2017-10-05T20:53:10Z

docs/api.md

+    + [executionContext.evaluate(pageFunction, ...args)](#executioncontextevaluatepagefunction-args)
+    + [executionContext.object(pageFunction, ...args)](#executioncontextobjectpagefunction-args)
+  * [class: ObjectHandle](#class-objecthandle)
+    + [objectHandle.asArray()](#objecthandleasarray)


Use getProperties instead.

pavelfeldman · 2017-10-05T21:05:34Z

docs/api.md

+  * [class: ExecutionContext](#class-executioncontext)
+    + [executionContext.evaluate(pageFunction, ...args)](#executioncontextevaluatepagefunction-args)
+    + [executionContext.object(pageFunction, ...args)](#executioncontextobjectpagefunction-args)
+  * [class: ObjectHandle](#class-objecthandle)


As discussed offline, renamed into JSHandle

pavelfeldman · 2017-10-05T21:06:08Z

docs/api.md

+    + [objectHandle.asObject()](#objecthandleasobject)
+    + [objectHandle.context()](#objecthandlecontext)
+    + [objectHandle.dispose()](#objecthandledispose)
+    + [objectHandle.get(propertyName)](#objecthandlegetpropertyname)


getProperty

pavelfeldman · 2017-10-05T21:07:59Z

docs/api.md

+    + [objectHandle.dispose()](#objecthandledispose)
+    + [objectHandle.get(propertyName)](#objecthandlegetpropertyname)
+    + [objectHandle.toString()](#objecthandletostring)
+    + [objectHandle.type()](#objecthandletype)


Remove type, I would add evaluate.

Killed type; as per offline discussion, holding off from adding .evaluate in this patch until its necessary

a1ph · 2017-10-05T20:59:54Z

docs/api.md

+- `...args` <...[Serializable]|[ObjectHandle]> Arguments to pass to `pageFunction`
+- returns: <[Promise]<[ObjectHandle]>> Resolves to the return value of `pageFunction`
+
+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.


a1ph · 2017-10-05T21:00:09Z

docs/api.md

+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.
+
+```js
+const aHandle = await page.object(() => {


a1ph · 2017-10-05T21:02:50Z

docs/api.md

+
+Returns either `null` or the object handle itself, if the object handle is an instance of [ElementHandle].
+
+#### objectHandle.asJSON()


I'd expect this method to return a string

Renamed into objectHandle.jsonValue()

a1ph · 2017-10-05T21:11:40Z

utils/doclint/check_public_api/MDBuilder.js

@@ -59,7 +59,7 @@ class MDOutline {
          let actualText = element.firstChild.textContent;
          let angleIndex = actualText.indexOf('<');
          let spaceIndex = actualText.indexOf(' ');
-          angleIndex = angleIndex === -1 ? angleText.length : angleIndex;
+          angleIndex = angleIndex === -1 ? actualText.length : angleIndex;
          spaceIndex = spaceIndex === -1 ? spaceIndex.length : spaceIndex + 1;


spaceIndex === -1 ? spaceIndex.length looks sketchy.

Thanks, done.

dgozman · 2017-10-05T21:29:23Z

ObjectHandle -> JSHandle
object -> evaluateHandle
get -> getProperty
asArray, asObject -> getProperties
asJSON -> jsonValue
handle.context().evaluate(h => h.arr[2].prop, handle) as a best-practice in docs?

aslushnikov

addressed all comments

aslushnikov · 2017-10-05T21:49:47Z

docs/api.md

@@ -55,6 +55,7 @@
    + [page.keyboard](#pagekeyboard)
    + [page.mainFrame()](#pagemainframe)
    + [page.mouse](#pagemouse)
+    + [page.object(pageFunction, ...args)](#pageobjectpagefunction-args)


aslushnikov · 2017-10-05T21:53:38Z

docs/api.md

@@ -109,21 +110,44 @@
    + [frame.$eval(selector, pageFunction[, ...args])](#frameevalselector-pagefunction-args)
    + [frame.addScriptTag(url)](#frameaddscripttagurl)
    + [frame.childFrames()](#framechildframes)
+    + [frame.context()](#framecontext)


aslushnikov · 2017-10-05T21:53:40Z

docs/api.md

    + [frame.evaluate(pageFunction, ...args)](#frameevaluatepagefunction-args)
    + [frame.injectFile(filePath)](#frameinjectfilefilepath)
    + [frame.isDetached()](#frameisdetached)
    + [frame.name()](#framename)
+    + [frame.object(pageFunction, ...args)](#frameobjectpagefunction-args)


aslushnikov · 2017-10-05T21:55:34Z

docs/api.md

+  * [class: ExecutionContext](#class-executioncontext)
+    + [executionContext.evaluate(pageFunction, ...args)](#executioncontextevaluatepagefunction-args)
+    + [executionContext.object(pageFunction, ...args)](#executioncontextobjectpagefunction-args)
+  * [class: ObjectHandle](#class-objecthandle)


As discussed offline, renamed into JSHandle

aslushnikov · 2017-10-05T22:01:28Z

docs/api.md

+  * [class: ObjectHandle](#class-objecthandle)
+    + [objectHandle.asArray()](#objecthandleasarray)
+    + [objectHandle.asElement()](#objecthandleaselement)
+    + [objectHandle.asJSON()](#objecthandleasjson)


aslushnikov · 2017-10-05T22:22:53Z

utils/doclint/check_public_api/MDBuilder.js

@@ -59,7 +59,7 @@ class MDOutline {
          let actualText = element.firstChild.textContent;
          let angleIndex = actualText.indexOf('<');
          let spaceIndex = actualText.indexOf(' ');
-          angleIndex = angleIndex === -1 ? angleText.length : angleIndex;
+          angleIndex = angleIndex === -1 ? actualText.length : angleIndex;
          spaceIndex = spaceIndex === -1 ? spaceIndex.length : spaceIndex + 1;


Thanks, done.

aslushnikov · 2017-10-05T22:23:17Z

docs/api.md

+
+Returns either `null` or the object handle itself, if the object handle is an instance of [ElementHandle].
+
+#### objectHandle.asJSON()


Renamed into objectHandle.jsonValue()

aslushnikov · 2017-10-05T23:14:40Z

docs/api.md

+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.
+
+```js
+const aHandle = await page.object(() => {


aslushnikov · 2017-10-05T23:35:26Z

docs/api.md

+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.
+
+```js
+const aHandle = await page.object(() => {


aslushnikov · 2017-10-05T23:36:07Z

docs/api.md

+- `...args` <...[Serializable]|[ObjectHandle]> Arguments to pass to `pageFunction`
+- returns: <[Promise]<[ObjectHandle]>> Resolves to the return value of `pageFunction`
+
+If the function, passed to the `page.object`, returns a [Promise], then `page.object` would wait for the promise to resolve and return it's value.


pavelfeldman

lgtm

This patch: - introduces ExecutionContext class that incapsulates javascript execution context. An examples of execution contexts are workers and frames - introduces ObjectHandle that holds a references to the javascript object in ExecutionContext - inherits ElementHandle from ObjectHandle Fixes puppeteer#382.

JoelEinbinder · 2017-10-06T19:21:25Z

docs/api.md

+A string can also be passed in instead of a function.
+
+```js
+const aHandle = await page.evaluateHandle('document'); // Handle on the 'document'.


Handle of the 'document'

Went on with "for".

JoelEinbinder · 2017-10-06T19:24:25Z

docs/api.md

@@ -1314,8 +1365,129 @@ puppeteer.launch().then(async browser => {
 });
 ```

+### class: ExecutionContext
+
+The class represents a context for javascript execution. Examples of javascript contexts are:


nit: JavaScript

JoelEinbinder · 2017-10-06T19:24:49Z

docs/api.md

+### class: ExecutionContext
+
+The class represents a context for javascript execution. Examples of javascript contexts are:
+- each frame has a separate execution context


Link to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe

JoelEinbinder · 2017-10-06T19:25:18Z

docs/api.md

+
+The class represents a context for javascript execution. Examples of javascript contexts are:
+- each frame has a separate execution context
+- all kind of workers have their own contexts


Link to https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API

JoelEinbinder · 2017-10-06T19:27:48Z

docs/api.md

+console.log(await executionContext.evaluate('1 + 2')); // prints "3"
+```
+
+[JSHandle] instances could be passed as arguments to the `frame.evaluate`:


~~could~~ can

Let's try to use a consistent tense.

Replaced here for now. Probably will replace everywhere later.

JoelEinbinder · 2017-10-06T19:47:15Z

lib/ExecutionContext.js

+  async getProperties() {
+    const response = await this._client.send('Runtime.getProperties', {
+      objectId: this._remoteObject.objectId,
+      ownProperties: true


Why do we want only enumerable and "own" properties? Feels arbitrary.

Non-enumerable might clash with names.
ownProperties is a good call, I removed this flag and added a test.

JoelEinbinder · 2017-10-06T19:50:15Z

lib/ExecutionContext.js

+    await helper.releaseObject(this._client, this._remoteObject);
+  }
+
+  /**


JoelEinbinder · 2017-10-06T19:51:23Z

lib/ElementHandle.js

      if (!element.ownerDocument.contains(element))
        return null;
+      if (element.nodeType !== HTMLElement.ELEMENT_NODE)


Why are we making element handles for nodes?

There's no good way to differentiate for now; as discussed offline, I'll make them throw for elementHandle methods for now.

JoelEinbinder · 2017-10-06T19:52:38Z

lib/FrameManager.js

+    const context = this._contextIdToContext.get(contextId);
+    console.assert(context, 'INTERNAL ERROR: missing context with id = ' + contextId);
+    if (remoteObject.subtype === 'node')
+      return new ElementHandle(context, this._client, remoteObject, this._mouse, this._touchscreen);


Can we check if it is an element here, instead of just a node?

We can't: it requires either an async hop to the page, or breaking change to the protocol. We'd be very unhappy with both.

JoelEinbinder · 2017-10-06T19:53:43Z

utils/doclint/check_public_api/MDBuilder.js

@@ -59,8 +59,8 @@ class MDOutline {
          let actualText = element.firstChild.textContent;
          let angleIndex = actualText.indexOf('<');
          let spaceIndex = actualText.indexOf(' ');
-          angleIndex = angleIndex === -1 ? angleText.length : angleIndex;
-          spaceIndex = spaceIndex === -1 ? spaceIndex.length : spaceIndex + 1;
+          angleIndex = angleIndex === -1 ? actualText.length : angleIndex;


What does this fix?

There's no such thing as 'angleText'. This fixes a typo.

JoelEinbinder · 2017-10-06T19:54:19Z

test/test.js

+    it('should work for TextNodes', SX(async function() {
+      await page.goto(PREFIX + '/input/button.html');
+      const buttonTextNode = await page.evaluateHandle(() => document.querySelector('button').firstChild);
+      await buttonTextNode.click('button');


element.click doesn't take a string.

This lies about clicking the text node.

good call about a string. Regarding clicking text node: i switched to throwing in case of text nodes as we discussed offline.

JoelEinbinder · 2017-10-06T19:56:17Z

test/test.js

+      const [frame1, frame2] = page.frames();
+      expect(frame1.executionContext()).toBeTruthy();
+      expect(frame2.executionContext()).toBeTruthy();
+      expect(frame1.executionContext() !== frame2.executionContext()).toBeTruthy();


Can we evaluate something just so this test makes me feel safer?

Sure thing; done.

aslushnikov requested a review from pavelfeldman October 3, 2017 01:46

a1ph reviewed Oct 5, 2017

View reviewed changes

pavelfeldman reviewed Oct 5, 2017

View reviewed changes

a1ph reviewed Oct 5, 2017

View reviewed changes

aslushnikov force-pushed the object-handles branch from 974a350 to 64fcfba Compare October 5, 2017 23:54

aslushnikov commented Oct 5, 2017

View reviewed changes

pavelfeldman reviewed Oct 6, 2017

View reviewed changes

aslushnikov added 11 commits October 6, 2017 11:27

s/page.object/page.evaluateHandle/g

b95db6b

s/frame.context()/frame.executionContext()/g

b51b601

s/ObjectHandle/JSHandle/g

e65c761

s/asJSON/jsonValue/g

816d19e

s/objectHandle.context/objectHandle.executionContext/g

9e63e65

Introduce JSHandle.getProperty(-ies)

44c6fe7

kill JSHandle.type()

9207cab

address doclint comments

e5df97b

Update docs/api.md

7ed28d2

Do not change ConsoleMessage in this patch

a557255

aslushnikov force-pushed the object-handles branch from 64fcfba to a557255 Compare October 6, 2017 18:48

add JSHandle.toString() test

65d1a7c

JoelEinbinder reviewed Oct 6, 2017

View reviewed changes

aslushnikov added 2 commits October 6, 2017 14:21

address comments on documentation

f506946

Address Joel's comments

ddda822

JoelEinbinder approved these changes Oct 6, 2017

View reviewed changes

aslushnikov merged commit 0d0f9b7 into puppeteer:master Oct 6, 2017

JoelEinbinder mentioned this pull request Oct 6, 2017

fix(input): clicking an element should take into account frame position #971

Merged

aslushnikov deleted the object-handles branch October 10, 2017 00:18

		* [class: ElementHandle](#class-elementhandle)
		+ [elementHandle.asArray()](#elementhandleasarray)


		Returns either `null` or the object handle itself, if the object handle is an instance of [ElementHandle].

		#### objectHandle.asJSON()

feat(ObjectHandles): Implement Object Handles #943

feat(ObjectHandles): Implement Object Handles #943

Conversation

aslushnikov commented Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgozman commented Oct 5, 2017

aslushnikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelfeldman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov commented Oct 3, 2017 •

edited

aslushnikov Oct 6, 2017 •

edited