Adds a waitForXpath method to page and frame. #1767

fringd · 2018-01-10T22:14:45Z

Here's a waitForXpath function as per issue 1757

googlebot · 2018-01-10T22:14:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

fringd · 2018-01-10T22:18:19Z

I signed it!

googlebot · 2018-01-10T22:18:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

fringd · 2018-01-10T22:20:45Z

@aslushnikov please review!

googlebot · 2018-01-10T22:26:23Z

CLAs look good, thanks!

JoelEinbinder · 2018-01-10T22:42:14Z

docs/api.md

The // heuristic makes me a bit uncomfortable. It further fractures this method and doesn't include all xpath.

I think it's a pragmatic heuristic, and there's waitForXpath if you have something else. importantly anything starting with // is NOT valid css.

I don't feel strongly about it though, and I'm happy to just remove support for Xpath from waitFor if this is a blocker.

I thought of using xpath if it's valid xpath, or css if it's valid css, but there's some overlap there, and I'm not sure if the meaning would always be the same.

I'm open to any other alternatives you can think of as well, but I didn't come to this thoughtlessly.

sorry reading back my response, it sounds a bit douchy the next day. I appreciate your dedication to keeping the API clear and I'm happy to fix this PR until it is.

basically the only thing that will work without an inital // is something that starts with

"html/body/" and I don't think this is how people use xpath, but I might be wrong.

On a fresh look, I like this. Thanks!

JoelEinbinder · 2018-01-10T22:54:14Z

lib/FrameManager.js

I'm not super familiar with XPath, but maybe we want Xpath.FIRST_ORDERED_NODE_TYPE to get some consistency with the results.

JoelEinbinder · 2018-01-10T23:03:49Z

lib/FrameManager.js

This has a lot of code shared with waitForSelector. I think we should pop it out into its own function, and add an xpath/selector argument to it.

removed some duplication.

JoelEinbinder · 2018-01-10T23:04:11Z

lib/FrameManager.js

Good catch.

JoelEinbinder · 2018-01-10T23:05:45Z

test/test.js

If we share the same predicate code with waitForSelector, I think a lot of these tests become redundant.

I've changed the code to be shared. Which ones do you think we should keep?

Okay I've used my best judgement and removed most of them.

JoelEinbinder · 2018-01-10T23:07:30Z

Thanks for the PR! Looks good!

vsemozhetbyt · 2018-01-11T00:39:08Z

docs/api.md

The bottom reference for [xpath] link is missing.

ah, missed that. added the link.

googlebot · 2018-01-11T00:41:17Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

vsemozhetbyt · 2018-01-11T00:45:14Z

docs/api.md

Typo in the hash:

#pagewaitforsxpathxpath-options -> #pagewaitforxpathxpath-options ------------^

you are eagle-eyed sir. fixed.

googlebot · 2018-01-11T00:54:21Z

CLAs look good, thanks!

vsemozhetbyt · 2018-01-11T00:59:56Z

docs/api.md

Duplicate sentences)

vsemozhetbyt · 2018-01-11T01:01:19Z

docs/api.md

The same typo)

vsemozhetbyt · 2018-01-11T01:04:01Z

docs/api.md

Is the comma intended (here and in the page counterpart)?

fringd · 2018-01-12T17:37:32Z

hey just following up on this branch, don't want it to get forgotten. I believe I've responded to all the excellent feedback, PTAL

fringd · 2018-01-12T19:45:24Z

@vsemozhetbyt @JoelEinbinder ^^

vsemozhetbyt · 2018-01-12T20:37:56Z

@fringd Sorry, I am not a maintainer, just a contributor like you)

fringd · 2018-01-16T17:18:07Z

@JoelEinbinder gentle ping, if there's more you want to get this merged, please let me know.

JoelEinbinder · 2018-01-17T08:36:32Z

docs/api.md

Nit: I think xpath is capitalized as XPath for the link, and that would make it waitForXPath as the method name.

JoelEinbinder · 2018-01-17T08:40:15Z

lib/FrameManager.js

I think these days ElementHandle is actually fine to use for nodes. We use it internally as a reference to the Document node. So this check can be removed.

Alright, fixed this code to work with text nodeds and adding a test to verify selecting textNode works

JoelEinbinder · 2018-01-17T08:43:06Z

lib/FrameManager.js

Seeing as we are using an imperfect heuristic, I think we can be more aggressive and say its an XPath if it just starts with /. string.startsWith('/') .

alright, SGTM

JoelEinbinder

Sorry for the delay! Looks good, with some small comments.

fringd · 2018-01-17T16:34:40Z

thanks for the review, and no problem about the wait. congrats on 1.0 BTW ^_^

fringd · 2018-01-19T17:16:20Z

@JoelEinbinder PTAL thx ^_^

googlebot · 2018-01-19T20:25:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-01-19T20:25:54Z

CLAs look good, thanks!

JoelEinbinder · 2018-01-20T00:39:24Z

yarn.lock

  version "2.0.2"
  resolved "https://registry.yarnpkg.com/esutils/-/esutils-2.0.2.tgz#0abf4f1caa5bcb1f7a9d8acc6dea4faaa04bac9b"

-exit@^0.1.2:


I don't think yarn.lock should change with this patch.

reverted this change.

googlebot · 2018-01-20T01:33:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-01-20T01:47:46Z

CLAs look good, thanks!

fringd · 2018-01-22T17:43:33Z

ping @JoelEinbinder I removed the yarn.lock changes. not sure how I altered those, It must have been some bad git rebasing.

aslushnikov · 2018-01-22T22:38:33Z

lib/FrameManager.js

  waitFor(selectorOrFunctionOrTimeout, options = {}, ...args) {
-    if (helper.isString(selectorOrFunctionOrTimeout))
-      return this.waitForSelector(/** @type {string} */(selectorOrFunctionOrTimeout), options);
+    const xpathPattern = /^\//;


let's check for double-slash here, otherwise it gonna be a breaking change.

Consider the following usecase:

const crazySelector = '/* comment */div'; await page.waitFor(crazySelector);

would that work?

crazy, fixed.

aslushnikov · 2018-01-22T22:39:08Z

lib/FrameManager.js

+
+    if (helper.isString(selectorOrFunctionOrTimeout)) {
+      const string = /** @type {string} */ (selectorOrFunctionOrTimeout);
+      if (string.match(xpathPattern))


nit: let's do string.startsWith instead

aslushnikov · 2018-01-22T22:40:43Z

lib/FrameManager.js

+      const string = /** @type {string} */ (selectorOrFunctionOrTimeout);
+      if (string.match(xpathPattern))
+        return this.waitForXPath(string, options);
+      else


style: drop else. (We follow blink's codestyle)

fringd · 2018-01-22T22:58:03Z

@JoelEinbinder @aslushnikov made some more changes in response to feedback.

aslushnikov

@fringd thank you very much for prompt responses!

fringd mentioned this pull request Jan 10, 2018

moar xpath! #1757

Closed

fringd force-pushed the xpath branch from 52cf36f to 956bbc8 Compare January 10, 2018 22:26

JoelEinbinder reviewed Jan 10, 2018

View reviewed changes

vsemozhetbyt reviewed Jan 11, 2018

View reviewed changes

fringd force-pushed the xpath branch from 11e45b3 to 3d3f789 Compare January 11, 2018 00:54

vsemozhetbyt reviewed Jan 11, 2018

View reviewed changes

docs/api.md Outdated

Copy link

Contributor

vsemozhetbyt Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same typo)

vsemozhetbyt reviewed Jan 11, 2018

View reviewed changes

docs/api.md Outdated

Copy link

Contributor

vsemozhetbyt Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comma intended (here and in the page counterpart)?

fringd force-pushed the xpath branch 2 times, most recently from 95b6ae3 to af5f403 Compare January 11, 2018 01:12

JoelEinbinder reviewed Jan 17, 2018

View reviewed changes

JoelEinbinder approved these changes Jan 17, 2018

View reviewed changes

Adds a waitForXpath method to page and frame.

d65ad2b

fringd force-pushed the xpath branch from af5f403 to d65ad2b Compare January 17, 2018 16:33

Xpath -> XPath

6acf2b7

fringd force-pushed the xpath branch from 3c3109a to 6acf2b7 Compare January 19, 2018 20:25

JoelEinbinder reviewed Jan 20, 2018

View reviewed changes

oops reverting yarn.lock

a97b9c5

fringd force-pushed the xpath branch from b1afe1b to a97b9c5 Compare January 20, 2018 01:47

aslushnikov suggested changes Jan 22, 2018

View reviewed changes

changes for aslushnikov

a962fa1

aslushnikov approved these changes Jan 22, 2018

View reviewed changes

aslushnikov merged commit cb684eb into puppeteer:master Jan 22, 2018

Adds a waitForXpath method to page and frame. #1767

Adds a waitForXpath method to page and frame. #1767

Uh oh!

Conversation

fringd commented Jan 10, 2018

Uh oh!

googlebot commented Jan 10, 2018

Uh oh!

fringd commented Jan 10, 2018

Uh oh!

googlebot commented Jan 10, 2018

Uh oh!

fringd commented Jan 10, 2018

Uh oh!

googlebot commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelEinbinder commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fringd commented Jan 12, 2018

Uh oh!

fringd commented Jan 12, 2018

Uh oh!

vsemozhetbyt commented Jan 12, 2018

Uh oh!

fringd commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!