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

Proposal: deprecate waitFor and add waitForTimeout #6214

Closed
jackfranklin opened this issue Jul 13, 2020 · 32 comments · Fixed by #6268
Closed

Proposal: deprecate waitFor and add waitForTimeout #6214

jackfranklin opened this issue Jul 13, 2020 · 32 comments · Fixed by #6268

Comments

@jackfranklin
Copy link
Collaborator

Puppeteer's API currently has a few waitForX functions:

  • waitForSelector
  • waitForFunction
  • waitForXPath

(And others that are less relevant to this proposal).

We also ship waitFor, which has the following docs:

This method behaves differently with respect to the type of the first parameter:

* if selectorOrFunctionOrTimeout is a string, then the first argument is treated as a selector or xpath, depending on whether or not it starts with '//', and the method is a shortcut for page.waitForSelector or page.waitForXPath
* if selectorOrFunctionOrTimeout is a function, then the first argument is treated as a predicate to wait for and the method is a shortcut for page.waitForFunction().
* if selectorOrFunctionOrTimeout is a number, then the first argument is treated as a timeout in milliseconds and the method returns a promise which resolves after the timeout
* otherwise, an exception is thrown

So if you call waitFor(someFunc), you're just calling waitForFunction. Similarly, waitFor('.foo') calls waitForSelector, and waitFor('//div') calls waitForXPath. The only unique behaviour waitFor provides is calling it with a time: waitFor(1000) does exactly what you expect.

Another downside of this method is that it's harder to type safely with TypeScript (you can do it via overloads, but it's uneccessarily complex, especially when all the standalone waitForX functions are already typed).

I think it's confusing that most of the API for waiting is explicitly named, e.g. waitForSelector, yet the only way to wait for a timeout is to call waitFor.

I'd propose a nicer, more consistent API would:

  1. Add waitForTimeout. This is consistent with the naming of the other wait for methods, which are all named waitForX.
  2. Remove waitFor.

Rather than do this as one breaking change we can do this change in two parts:

  1. Ship a version with waitForTimeout. Deprecate waitFor and log a message when it's used, but maintain its current behaviour.
  2. After a reasonable amount of time, ship a release that removes waitFor. The migration path should be straightforward, users will have to swap waitFor for the correct waitForX method. We'll have a waitForX for every behaviour of the original waitFor, so this won't be more than a renaming of some method calls.
@jackfranklin jackfranklin changed the title Proposal: deprecate waitFor and add waitForTime Proposal: deprecate waitFor and add waitForTimer Jul 13, 2020
@jackfranklin
Copy link
Collaborator Author

@mathiasbynens @hanselfmu @OrKoN PTAL

@mathiasbynens
Copy link
Member

LGTM. Explicit, dedicated APIs > generic overloaded APIs (even if you don't consider TypeScript).

@OrKoN
Copy link
Collaborator

OrKoN commented Jul 14, 2020

LGTM

@supnate
Copy link

supnate commented Sep 11, 2020

Please don't remove waitFor. We have a CI running tests from more than 40 repos from different teams in one job. If it's removed, all teams need to commit the update at the same time to make build success. It's a hard thing to do.

@jackfranklin
Copy link
Collaborator Author

@supnate waitFor isn't removed yet but we do plan to remove it in a future release. That said, it's not going to happen imminently so your teams have plenty of time. I'd suggest trying to migrate slowly to prepare yourself for when waitFor is removed, but rest assured you have time on your side 👍

@supnate
Copy link

supnate commented Sep 12, 2020

Thanks @jackfranklin for the suggestion. But you know people don't care warnings much. We can ask teams in different countries to do it but there will be no mechanism to ensure all code are updated before it's finally removed unless we write some script to check it. We've been using puppeteer very smoothly from 1.x to 5.x and we wish it could continue. like you can add a new API but just remove waitFor from docs but it never really deprecate.

@supnate
Copy link

supnate commented Sep 12, 2020

BTW, would you please update the issue title using waitForTimeout instead of waitForTimer since the warning points to this issue?

@delesseps
Copy link

Does someone know what the future of the TypeScript Types are? with waitFor being deprecated I don't see anyone looking to add waitForTimeout

currently if anyone is using TypeScript they can add the following to extend the definitions

import * as puppeteer from 'puppeteer';

declare module 'puppeteer' {
  export interface Page {
    waitForTimeout(duration: number): Promise<void>;
  }
}

adamdbradley added a commit to ionic-team/stencil that referenced this issue Sep 17, 2020
@fgroupindonesia
Copy link

fgroupindonesia commented Sep 18, 2020

and what about the usage of waitForX is that still same as waitFor original parameter?
and... is it reasonable for hiding the notification at the moment? :D

@Colton1070
Copy link

Yeah, I was confused being told it's deprecated but also the only way currently to wait for a specific amount of time is to use waitFor(1000). Hope either an alternate function is created or waitFor() becomes the time exclusive version.

swissspidy added a commit to GoogleForCreators/web-stories-wp that referenced this issue Oct 1, 2020
`waitFor` is deprecated and will be removed in a future release.

See puppeteer/puppeteer/issues/6214
swissspidy added a commit to GoogleForCreators/web-stories-wp that referenced this issue Oct 2, 2020
`waitFor` is deprecated and will be removed in a future release.

See puppeteer/puppeteer/issues/6214
macbre added a commit to macbre/phantomas that referenced this issue Oct 5, 2020
@dobesv
Copy link

dobesv commented Oct 8, 2020

You can also wait for a specific time outside the Page, e.g.

await Bluebird.delay(100)

@framerate
Copy link

framerate commented Feb 7, 2022

Take a look at #3347

// this test fails with a time out
await page.waitForSelector(buttonSelector) 
await page.click(buttonSelector)

// this one passes
await page.waitForSelector(buttonSelector)
await page.waitFor(1000)
await page.click(buttonSelector)

// so does this one
await page.waitForSelector(buttonSelector)
page.$eval(buttonSelector, elem => elem.click())

So if you remove waitFor without fixing waitForSelector() or .click() a lot of people's tests may break

EDIT: wrong copy/paste

@zeevgg
Copy link

zeevgg commented Feb 10, 2022

hey, started getting warnings about deprecation.
so what is the new way to use it?

@framerate
Copy link

what is the new way to use it?

Looks like its waitForTimeout() now

@adrianarandac
Copy link

TypeError: runner.waitForTimeout(_html selector_) is not a function
I'm trying to replace a waitFor and got this message. Is it shipped?

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 16, 2022

If you want to wait for a number of milliseconds to pass, use page.waitForTimeout(1000). If you want to wait for a selector, use page.waitForSelector(".selector").

Please see the documentation for these methods: https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#pagewaitfortimeoutmilliseconds and make sure you use a version that ships waitForTimeout.

@OrKoN OrKoN changed the title Proposal: deprecate waitFor and add waitForTimer Proposal: deprecate waitFor and add waitForTimeout Feb 16, 2022
@OrKoN OrKoN changed the title Proposal: deprecate waitFor and add waitForTimeout Proposal: deprecate waitFor and add waitForTimeout Feb 16, 2022
plocket added a commit to SuffolkLITLab/ALKiln that referenced this issue Feb 17, 2022
I'm thinking this is just going to be for v4. Not bothering with this for v3 unless we absolutely have to since none of the vulnerabilities are severe. My current rationale is that the more work we do to maintain 3, the less work we can do getting v4 ready for release. Ready to hear opinions.

- Close #164, update cucumber to v7
- Prepare for v8 of cucumber because I won't remember it later
- Close #394, update puppeteer
- Update our version of node (and that of our action that we'll run for other people's libs). [Addresses #393 so we can use the suffolk npm org package.]
- Use `npm audit` to fix the remaining vulnerabilities (now 0)
- [Remove package.json as discussed in #489 to align our tests' behaviors with those of our users.]

* Update action.yml node to v17

* Update from cucumber v6 to v7. See details.

See https://github.com/cucumber/cucumber-js/blob/main/docs/migration.md#migrating-to-cucumber-js-7xx

Only use cucumber setDefaultTimeout globally and use a shim that replicates the fix in v8 that lets you do custom timeouts more easily so we can still give enough time for steps that may need more time.

Use all caps for statuses.

Test screenshot step.

Btw, the cucumber test output visually looks a bit different now - when a scenario passes, all the steps pass too.

Sorry about the little comment changes, etc. Tried to remove a lot of those incidental unrelated changes.

* Update puppeteer to latest (13). See details below.

- page.waitFor -> page.waitForTimeout and page.waitForSelector (Got deprication notice. See puppeteer/puppeteer#6214.)
- remove removeEventListener (we'd need to change it to removeListener anyway - v4.0.0 and see https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#eventemitterremovelistenerevent-handler). For now we'll count on page close taking care of it, just in case removing it would prevent multiple-file-downloads.

* Update GitHub worflow node version, tweak changelog item order

* Fix npm audit vulnerabilities and update action.yml cucumber

* Pin the colors lib in action.yml

* Remove package-lock.json #489, use kiln v4 for users, CHANGELOG

* Fix custom timeout, remove duplicate report entry, as per review
plocket added a commit to SuffolkLITLab/ALKiln that referenced this issue Mar 2, 2022
Created log.txt and git hub artifact for reports. Closes #466.

* add log.txt for report messages

* create artifact for logs

* Update changelog

* Update our package's dependencies for v4 (#503)

I'm thinking this is just going to be for v4. Not bothering with this for v3 unless we absolutely have to since none of the vulnerabilities are severe. My current rationale is that the more work we do to maintain 3, the less work we can do getting v4 ready for release. Ready to hear opinions.

- Close #164, update cucumber to v7
- Prepare for v8 of cucumber because I won't remember it later
- Close #394, update puppeteer
- Update our version of node (and that of our action that we'll run for other people's libs). [Addresses #393 so we can use the suffolk npm org package.]
- Use `npm audit` to fix the remaining vulnerabilities (now 0)
- [Remove package.json as discussed in #489 to align our tests' behaviors with those of our users.]

* Update action.yml node to v17

* Update from cucumber v6 to v7. See details.

See https://github.com/cucumber/cucumber-js/blob/main/docs/migration.md#migrating-to-cucumber-js-7xx

Only use cucumber setDefaultTimeout globally and use a shim that replicates the fix in v8 that lets you do custom timeouts more easily so we can still give enough time for steps that may need more time.

Use all caps for statuses.

Test screenshot step.

Btw, the cucumber test output visually looks a bit different now - when a scenario passes, all the steps pass too.

Sorry about the little comment changes, etc. Tried to remove a lot of those incidental unrelated changes.

* Update puppeteer to latest (13). See details below.

- page.waitFor -> page.waitForTimeout and page.waitForSelector (Got deprication notice. See puppeteer/puppeteer#6214.)
- remove removeEventListener (we'd need to change it to removeListener anyway - v4.0.0 and see https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#eventemitterremovelistenerevent-handler). For now we'll count on page close taking care of it, just in case removing it would prevent multiple-file-downloads.

* Update GitHub worflow node version, tweak changelog item order

* Fix npm audit vulnerabilities and update action.yml cucumber

* Pin the colors lib in action.yml

* Remove package-lock.json #489, use kiln v4 for users, CHANGELOG

* Fix custom timeout, remove duplicate report entry, as per review

* Allow a developer to wait as a first Step v4. #387. Add test. (#506)

Closes #387.

Also, generally adds safety measures for when page does not exist.

Very similar to PR #459, but moving the responsibility down to a spot that most functions make use of, meaning that it'll be applied to a lot more cases. They mostly won't need it, but it might still be worth being more comprehensive.

* Allow a developer to wait as a first Step v4. #387. Add test.

Will be able to close once we've added this as an establishing
step (in addition to it being a regular step).

Also, generally adds safety measures for when page does not exist.

* Add test

Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>

Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>

* add log to gitnore and cleanup console.logs and typos

* add empty string to file

Co-authored-by: plocket <52798256+plocket@users.noreply.github.com>
Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>
CvX added a commit to discourse/discourse that referenced this issue May 3, 2022
1. Updates puppeteer to x
2. Fixes deprecations:
    ```
    waitFor is deprecated and will be removed in a future release. See puppeteer/puppeteer#6214 for details and how to migrate your code.
    ```
3. Lints/prettyfies the smoke_test.js file
CvX added a commit to discourse/discourse that referenced this issue May 3, 2022
1. Updates puppeteer to x
2. Fixes deprecations:
    ```
    waitFor is deprecated and will be removed in a future release. See puppeteer/puppeteer#6214 for details and how to migrate your code.
    ```
3. Lints/prettyfies the smoke_test.js file
outsideris added a commit to outsideris/mocha-examples that referenced this issue May 25, 2022
ref: puppeteer/puppeteer#6214

Signed-off-by: Outsider <outsideris@gmail.com>
outsideris added a commit to outsideris/mocha-examples that referenced this issue May 29, 2022
ref: puppeteer/puppeteer#6214

Signed-off-by: Outsider <outsideris@gmail.com>
outsideris added a commit to mochajs/mocha-examples that referenced this issue May 29, 2022
ref: puppeteer/puppeteer#6214

Signed-off-by: Outsider <outsideris@gmail.com>
asbjornu added a commit to asbjornu/jsonresume-theme-golden that referenced this issue May 7, 2023
- Install percy as a development dependency instead of installing it
  as a separate build step.
- [Replace deprecated `waitFor` with `waitForTimeout`][1].
- Explicitly add `--theme .` argument to the `npx resume` command.

[1]: puppeteer/puppeteer#6214
asbjornu added a commit to asbjornu/jsonresume-theme-golden that referenced this issue May 7, 2023
- Install percy as a development dependency instead of installing it
  as a separate build step.
- [Replace deprecated `waitFor` with `waitForTimeout`][1].
- Explicitly add `--theme .` argument to the `npx resume` command.

[1]: puppeteer/puppeteer#6214
@San-Zer0-786
Copy link

TypeError: page.waitForTimeout is not a function

TypeError: page.waitFor is not a

This is on latest 22.3.0

How annoying. What harm is there in keeping older function names, to prevent projects from being nuked on updates?

@civilizator
Copy link

Same thing, although it worked fine for two years TypeError: page.waitForTimeout is not a function puppeteer 22.4.0

Updated to version 22.4.1 and problem still persists.

Downgrade to 20.3.0 and works great for me.

@antonok-edm
Copy link

In case anyone else finds this issue wondering why neither waitFor nor waitForTimeout exist - see #11780.

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 a pull request may close this issue.