refactor(waitFor): replaced page.waitFor by its successor page.waitForTimeout

[mfranzke]

waitFor is deprecated and will be removed in a future release. See puppeteer/puppeteer#6214 for details and how to migrate your code.

Fixes #1276

[garris]

Thank you!

[gruberro]

@garris, @mfranzke IMHO this MR will introduce problems with most existing BackstopJS configurations. waitForTimeout is not a successor of waitFor in Puppeteer, but is one of the specialized waitForX-methods offered by Puppeteer. waitFor is a - now deprecated - helper method encapsulating all the specialized waitForX-methods (timeout, selector, xPath and function). The new waitForTimeout is accepting a timeout (as number) only! Whilst the BackstopJS API still allows and documents to pass either a selector, an xPath, a timeout or a callback/function.

In order to ensure BC with the current configuration API, the deprecated waitFor should be re-implemented in BackstopJS to encapsulate the more specific calls to the appropriate waitForX-methods of Puppeteer. I don't see another way to get rid of all the deprecation warnings. I'd be happy to contribute the necessary change - in case you agree 😄.

[mfranzke]

@gruberro, good catch, thanks a lot. Looks like my quick research on resolving the topic lead to a misleading solution/explanation. And I do like your solution, and I‘m curious about @garris opinion.

[garris]

Yes -- I agree with the sentiment here. Thanks all for taking the closer look. Are you all sure there is no 1:1 waitFor replacement currently? It sounds like we will simply be recreating it. Since there will still be backstop implementations using waitFor we should probably revert this last change and just overload puppeteer waitFor with this new helper?

Please feel free to propose code changes.

Thank you!

[mfranzke]

@garris I think that this comment out of the puppeteer ticket that @gruberro mentioned sums it up:

This PR adds page.waitForTimeout (and subsequently, frame.waitForTimeout) and deprecates the page.waitFor and frame.waitFor methods.
None of the functionality that waitFor provided is lost; it's now all available via the more explicit waitForX functions. This PR doesn't remove waitFor, all its functionality is still available. I'd suggest we ship this as a minor release, give it a while for users to adjust and then remove waitFor in the next reasonable breaking release.

So regarding your summary, I think you're exactly right:

It sounds like we will simply be recreating it. Since there will still be backstop implementations using waitFor we should probably revert this last change and just overload puppeteer waitFor with this new helper?

We would be recreating a method, that has been provided by puppeteer previously, which have decided that the user should call the more explicit waitForX functions in the future directly.

After reading through the mentioned (and the original) ticket, they state their approach quite clearly - probably it was too late when I did that in the first place to understand the necessary actions and follow them accurately, sorry for that.
Our target should be to prevent any confusion for others as well and better guide the BackstopJS users specifically regarding the migration than the general one provided by puppeteer so far, which has the following output:

waitFor is deprecated and will be removed in a future release. See puppeteer/puppeteer#6214 for details and how to migrate your code.

• So regarding backwards compatibility and to prevent that puppeteers deprecation warnings we would be recreating that waitFor helper function in the BackstopJS codebase, and decide on how to inform the BackstopJS users regarding their migration in the console, as well as leading them to a further enhanced BackstopJS documentation regarding those waitForX functions (see next topic).
  Regarding the code lifecycle of that recreated waitFor function in the BackstopJS codebase, we need to decide whether it's a permanent or temporary one - the latter would be the better approach from my point of view to be fully aligned with puppeteers API; so regarding its deprecation and removal as well, it would be to stick to puppeteers approach even also for that waitForX function within BackstopJS codebase: "I'd suggest we ship this as a minor release, give it a while for users to adjust and then remove waitFor in the next reasonable breaking release."
• But within the documentation as well as the files that I've changed with this merge request earlier, it would be to mention and implement the correct waitForX functions even already.

Everything out of my summary might have been already clear to everyone else, but I wanted to answer your question @garris as well as provide a list of the necessary tasks (especially regarding documentation) so that we're all on the same page.
What do you think, @gruberro? And could I support you on either the docs or the changes within the files out of this change request?

[fuhlig]

tldr:

• refactor waitForX calls in backstop
• do not provide fallbacks / function overloads
• if there is a breaking change by upgrading puppeteer, create a major release for backstopjs

Do you really think it is necessary to provide a fallback? I think this is extra effort due to implementation & maintainance and potentially error prone.
With the recent dependency update from puppeteer@2x to v5 the deprecation was introduced by puppeteer.

Puppeteer already provides a deprecation message to (backstopjs) users which is sufficient IMO.

waitFor will work atleast until puppeteer@6 and if this would be a breaking change to backstop users, I do not see a reason why a dependency update should not cause a major release for backstop. I am quite certain major releases by puppeteer might cause incompatibilities. It should be a goal to keep backstopjs up to date with puppeteer.

[fuhlig]

...Speaking of puppeteer upgrades

https://github.com/puppeteer/puppeteer/releases (2 major releases within 24h 🤔)

waitFor still works, incl. deprecation warning

[fuhlig]

Should page.waitForSelector be used here?

[gruberro]

That indeed not backward compatible, if someone is using an xpath expression rather then a CSS selector, this previously worked. But would no longer work afterwards.

[gruberro]

Thank you for all your mentions, all of them are valid. In the end we have to distinguish two approaches:

Backward compatible change

Since BackstopJS users today are allowed to use a CSS selector, an xPath expression or a function for all different configuration options available (e.g. hoverSelectors, readySelector, hideSelectors, postInteractionWait (also valid with a timeout as ms), ...), we could recreate waitFor in BackstopJS and use it internally. By doing so, all noisy warnings of Puppeteer regarding the deprecation of waitFor are gone and no new major release would be necessary.

Backward incompatible change

If you prefer a solution without magic behind the scenes, a more cleaner approach could be to stick each BackstopJS configuration option to a dedicated Puppeteer call (e.g. hoverSelector => page.waitForSelector). This may lead to some more/additional configuration options, since it might be relevant to have a hoverSelector as well as a hoverXpath. At least someone out there might use it already today 😄. This - end the end - would lead as you @fuhlig mention to a new major version of BackstopJS with some sort of migration path/documentation/...

Both of them are valid and technically possible. In the end you @garris should decide which approach is the one to go 😃.

There is - for sure - one remaining option (but for sure not the best one): we try to do our best to ignore the noisy deprecation warnings while running tests 🙈 and take care of the problem once waitFor is really removed from Puppeteer.

[fuhlig]

Thanks for elobarating further on this @gruberro

As puppeteer is the underlaying technology to control the browsers, I favor a solution leaning on puppeteer rather than customized, non "standard" (non puppeteer) APIs. Keeping up to date with puppeteer should be the way to go.
I almost dropped backstopjs altogether from testing due to the outdated puppeteer version as it is used a very old chrome version.

Keeping it backwards compatible for backstopjs users is ofcourse an argument. If a "fallback layer" is implemented, I'd recommend to make it pluggable and offer it only for one major version and then remove it for the next one. But then again - is it worth the effort?
Major versions include breaking changes. Masking compat issues by providing fallbacks usually result in more effort and bigger issues in the end.

[garris]

@fuhlig I think you have expressed the right way to think about it. In the past we've done some things to help bridge gaps when there are breaking changes in dependencies -- but you're right -- it rarely eliminates pain.

The least effort for backstop in this case is to just revert the last change and allow devs to upgrade at their own pace. If someone still wants to build a shim as an engine script to add back the syntactic sugar then I'd be happy to link to their GitHub or blog post -- this is a harmless way to enable most devs and to avoid the divergence liability.

[gruberro]

Sounds good to me! TY @mfranzke, @fuhlig and @garris!

[fuhlig]

Created a PR to upgrade puppeteer #1284