TypeScript: ElementArrayFinder::filter uses `wdpromise` instead of a standard `Promise`, breaking backwards compatibility

[jan-molak]

Protractor 5.1.0
File: element.ts:232

---

Hey @sjelin!

It seems like the change in the signature of the ElementArrayFinder::filter introduced in commit 995b1466, breaks some of the more advanced filter functions.

Changing the definition of the filterFn from

(element: ElementFinder, index?: number) => boolean | wdpromise.Promise<boolean>

to

(element: ElementFinder, index?: number) => boolean | Promise<boolean>

or even

(element: ElementFinder, index?: number) => boolean | PromiseLike<boolean>

should fix the problem.

---

The problem in more detail.

From what I can see, the signature has changed from:

filter(
    filterFn: (element: ElementFinder, index?: number) => any
): ElementArrayFinder

to

filter(
    filterFn: (element: ElementFinder, index?: number) => boolean | wdpromise.Promise<boolean>
): ElementArrayFinder

This breaks backwards compatibility and forces any existing filterFns to now return a wdpromise.Promise, rather than a standard Promise or even better - a PromiseLike.

One example of such filterFn, coming from the Serenity/JS code base, could look like this:

const hasRequiredText      = (option: ElementFinder) => option.getText().then(value => !!~this.values.indexOf(value)),
      isAlreadySelected    = (option: ElementFinder) => option.isSelected(),
      ensureOnlyOneApplies = (list: boolean[]) => list.filter(_ => _ === true).length === 1,
      select               = (option: ElementFinder) => option.click();

const optionsToClick = (option: ElementFinder) => Promise.all([
        hasRequiredText(option),
        isAlreadySelected(option),
    ])
    .then(ensureOnlyOneApplies);

element(by.css('select'))
    .all(by.css('option'))
    .filter(optionsToClick)            // <- here's the trouble
    .each(select);

As you can see, the signature of the optionsToClick function above is

(element: ElementFinder) => Promise<boolean>

which, even though working perfectly fine with Protractor 5.0.0, no longer works with 5.1.0.

Looking forward to hearing your thoughts!

Best,
Jan

[sjelin]

Sorry for the slow response, I've been very busy wrapping things up before I transfer to a new team.

Here's the thing: you really should be returning a wdpromise.Promise. The Serenity/JS example you've shown me is actually wrong (they should be using protractor.promise.all instead of Promise.all). Using a different promise implementation could cause control flow problems and introduce race conditions. So a type error is warning you about a real problem in your code. If you don't care, use a cast to make it go away.

That said, @types/selenium-webdriver needs to publish types for selenium-webdriver@^3.0.0, which should include the option to use any promise-likes in these situations when the control flow is disabled.

[jan-molak]

No worries, that's a shocker, I'm sad to see you go! Hope the new team is fun :-)

To address the issue you've raised - I don't think using Promise.all is a problem there, since Serenity/JS wraps the whole promise chain into a webdriver.Promise anyway, so it's executed within the control flow.

Using standard promises also allows for chaining webdriver and es6 promise-based library calls together (which I don't believe need to be themselves wrapped in webdriver.promises, since a standard promise chained onto a webdriver promise will not be executed until the webdriver one is completed from what I've witnessed). But if I missed anything, please do let me know! :-)

On your second point - is there any ticket related to @types/selenium-webdriver publishing the changes you mentioned? Do I understand correctly that they've already been implemented and only need to be published?

Thanks!
Jan

[sjelin]

I've had issues with native promises wrapped in webdriver promises in the past. From Protractor's point of view, it's best to just have this be a type error for now.

The selenium-webdriver@^3.0.0 is implemented, but its types are not. The selenium team doesn't maintain the @types, although the comments in their source code have very clear jsdoc types. If you want to be a hero you could update the @types for selenium-webdriver yourself.

[massimocode]

I'm not so convinced that sticking with the wdpromise.Promise is not the best course of action. I've raised an issue #4058 which shows that async/await is broken because of this. We've had to force version 5.0.0 to prevent this breaking change.

[jan-molak]

I've pinned Serenity/JS peer dependency on Protractor to 5.0.0 as well, until I have some time to investigate @sjelin's suggestion further.

@massimocode - I'm curious, how does the issue affect async/await?

[sjelin]

Indeed, async/await returns a native promise.

I want to be clear: A bug I encountered while trying to support async/await was actually the motivation behind sticker types for promises: SeleniumHQ/selenium#3037

You don't have to pin yourself to 5.0.0 to get around this problem - you could just use type casting. But I want to be clear: you are playing with fire here. For instance:

els.map(async function clickFooBars(el: ElementFinder): boolean {
  if (await el.getAttribute('foo') == 'bar') {
    // Because of the native promise generated by async/await, we are now in a separate
    // control flow task
    el.click();
  }
}).each((el) {
  expect(el).toHaveBeenClicked(); // Race condition here
})

Obviously that's map and not filter, and you shouldn't be doing clicks in filters, but people do all sorts of things.

You could argue that the issue is just that clickFooBars should have done await el.click();. In some sense, you'd be right. Because Protractor doesn't internally rely on the control flow working properly, and because at the end of filter the final results are put back on the control flow, as long as your filter function chains all its promises together you can probably get away with native promises. But you're relying on the specifics of Protractor's internal implementation here. I believe you're also relying on undefined behavior in the WebDriver control flow as well. The only 100% safe way to make sure you're not breaking the control flow is to stick to webdriver promises.

[jan-molak]

Thanks for the link to SeleniumHQ/selenium#3037, @sjelin. That's a very interesting conversation indeed!

After further investigation I decided to provide the filter method with a protractor.promise.all, as per your suggestion. That makes perfect sense as the input fed into filter is the result of a getText and isSelected, which both return a webdriver.promise, so the whole chain remains consistent.
Since the change only had to be applied in the local scope, I could also avoid leaking the webdriver.promise abstraction.

I think that your observations from SeleniumHQ/selenium#3037 make perfect sense - mixing up sync and async styles can be problematic. However, since in Serenity/JS we chain the scheduling of a native promise, rather than the already started promise, onto a webdriver.promise and link them all together, we can get away without synchronisation problems. As per @jleyba's comment:

You can use managed promises alongside native promises, you just can't mix async and sync styles. If you want to use them together, you need to treat managed promises as native: explicitly return to ensure things are properly linked.

Thanks for your help, it's been a very useful conversation!

[sjelin]

It's true, Protractor is perhaps a little overly strict here. But if you're using native promises, you should really turn the control flow off. And with the control flow off, ManagedPromise will be replaced with native promises. @types/selenium-webdriver should theoretically react to the control flow being turned off, though that's not implemented yet. Once that's implemented, you'll only run into this problem if the control flow is enabled but you're returning a non-ManagedPromise to filter, which really implies that you're mixing sync/async styles.

[massimocode]

@sjelin I noticed a missing await in the example code you posted on this comment #4049 (comment)

I think you meant to do await el.click() because click returns a promise that indicates the click has been performed.

[massimocode]

I was looking to move away from protractor and use webdriverjs natively, because after much usage I believe protractor actually causes more pain than it does good.

I noticed this: https://github.com/SeleniumHQ/selenium/wiki/WebDriverJs#moving-to-asyncawait

It seems as though Webdriver JS will very soon be deprecating the terrible promise manager and asking people to use async/await instead. For that reason, I think the protractor team should not be using webdriver promises in the signatures of methods. This is of course aside from the fact that I already showed how the example code given above trying to show the drawbacks of not using webdriver promises wasn't written correctly. If you're going to use async/await then you should use it, not randomly forget to apply it!