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

Implement new async API #6954

Merged
merged 10 commits into from
Jul 28, 2021
Merged

Implement new async API #6954

merged 10 commits into from
Jul 28, 2021

Conversation

christian-bromann
Copy link
Member

Proposed changes

In #6702 we've evaluated what is the best path forward after it was clear that fibers support would not continue with Node.js v16 and up. As a first step this patch improves the async API by minimizing the amount of await necessary. Today our async API has no support for element chaining, requiring users to do things like that:

const tagName = await (await $('foobar')).getTagName()

This patch adds a much nicer handling of these chained calls and allows things as follows:

const tagName = await $('body').$$('div').get(2).getTagName()
const elem = await $('body').$('header').$$('div').find(
    async (elem) => {
        const loc = await elem.getLocation()
        return loc.x === 30
    }
)
console.log((await elem.getLocation()).x) // 30
console.log(await $('body').$$('div').length) // 5

This change should be backwards compatible as the behavior stays the same when an element promise is resolved before calling a command on it. It will be also possible for users to have sync code with async code mixed, e.g.:

describe('webdriver.io page', () => {
    it('should run async', async () => {
        await browser.url('https://webdriver.io')
        console.log('async result', await $('div').$('div').getTagName())
    })

    it('should run sync', () => {
        browser.url('https://webdriver.io')
        console.log('sync result', $('div').$('div').getTagName())
    })
})

This also simplifies transitioning between both modes.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

This patch requires some more work and I will also add some additional documentation changes to have all code examples be async by default with a sync option to switch.

Reviewers: @webdriverio/project-committers


function wrapElementFn (promise: Promise<Clients.Browser>, cmd: Function, args: any[], prevInnerArgs?: { prop: string, args: any[] }): any {
return new Proxy(
Promise.resolve(promise).then((ctx: Clients.Browser) => cmd.call(ctx, ...args)),
Copy link
Member

Choose a reason for hiding this comment

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

Would this work as well? If so, do we need to try/catch this?

Suggested change
Promise.resolve(promise).then((ctx: Clients.Browser) => cmd.call(ctx, ...args)),
promise.then((ctx: Clients.Browser) => cmd.call(ctx, ...args)),

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, because we don't know if promise is actually a promise, so to be sure we wrap it this way

Copy link
Member

@erwinheitzman erwinheitzman left a comment

Choose a reason for hiding this comment

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

Added some comments, great changes to see!

@mgrybyk
Copy link
Member

mgrybyk commented Jun 11, 2021

maybe it makes sense to completely get rid of everything related to wdio-sync and implemented Chainable Promise API as the only standard?

In case there will appear some way to mimic sync behavior I'd better implement logic for it from scratch.
We have lot's of hacking tricks to make wdio-sync work and I believe this is a good time to make it finally happen

@christian-bromann
Copy link
Member Author

@mgrybyk as much as I would like to get rid of it we have the majority of users that use sync mode and also like it. Many probably wouldn't like to rewrite their whole test suite so we will have to stick with it until deprecation of Node v16

@mgrybyk
Copy link
Member

mgrybyk commented Jun 11, 2021

Sorry to say you are right even though it's easier to maintain single official design.
I guess we are stuck till 2023-04-30

@klamping
Copy link
Contributor

klamping commented Jul 9, 2021

Anything I can do to help move this PR forward? Is more testing needed?

@christian-bromann
Copy link
Member Author

@klamping I was a bit stuck on the TS implementation but @mgrybyk helped me out so I can move forward with this soonish.

@klamping
Copy link
Contributor

@christian-bromann Good to hear! Let me know if any testing is needed and I'll do my best to help out.

@mgrybyk
Copy link
Member

mgrybyk commented Jul 21, 2021

Is it possible to access elements array by index instead of find, ex await $('foo').$$('bar')[2].getTagName() ?

@christian-bromann
Copy link
Member Author

Is it possible to access elements array by index instead of find, ex await $('foo').$$('bar')[2].getTagName() ?

I don't think so, would that even be possible in general? Given that accessing an index is not calling a function that you can chain I am not sure. This is why this patch proposes a get method to do:

await $('foo').$$('bar').get(2).getTagName()

@mgrybyk
Copy link
Member

mgrybyk commented Jul 21, 2021

@christian-bromann yes, it's possible with Proxy :)

@christian-bromann
Copy link
Member Author

Cool, let me try that. I definitely prefer using indexes rather than introducing a get command.

@christian-bromann christian-bromann added PR: New Feature 🚀 PRs that contain new features needs-review These Pull Requests require review from project members and removed work-in-progress labels Jul 22, 2021
christian-bromann and others added 7 commits July 28, 2021 11:26
* update API template to add sync depcrecation warning

* rewrite API examples

* update guide section examples

* fix example

* fix xpath code

* update section

* improve design of depcrecation warning

* Update website/docs/SyncVsAsync.md

Co-authored-by: Erwin Heitzman <15839059+erwinheitzman@users.noreply.github.com>

* TypeScript support for new Async API (#7000)

* base implementation

* define iterator methods

* throw error if element index is out of bounds

* type improvements

* don't use async types for sync def

* add typing tests

* update deps

* fix build

* use rimraf to remove node_modules

* revert package-lock changes

* more type fixes

* fix unit tests

* revert package-lock changes

* fix call command

* attempt to get build fixed

* just call npm test

* try a different approach

* fix typings

* fix typing test

* Async API blog post (#7181)

* Blog post with announcement on improved async api

* PR feedback

* update boilerplate examples (#7192)

Co-authored-by: Erwin Heitzman <15839059+erwinheitzman@users.noreply.github.com>
@christian-bromann christian-bromann merged commit f9eae7a into main Jul 28, 2021
@christian-bromann christian-bromann deleted the cb-partial-sync-remove branch July 28, 2021 09:36
@alexmi256
Copy link
Contributor

Is this fully implemented in 7.9.0?
I'm getting:

TypeError: elem[prop] is not a function
    at /.../node_modules/@wdio/utils/build/shim.js:197:38
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
   ...
    at Context.executeAsync (/.../node_modules/@wdio/utils/build/shim.js:265:16)
    at Context.testFrameworkFnWrapper (/.../node_modules/@wdio/utils/build/test-framework/testFnWrapper.js:52:18)

When using something like:

const elem = await $('foo').$$('bar').get(0);

Maybe I'm using this the wrong way.

@christian-bromann
Copy link
Member Author

const elem = await $('foo').$$('bar').get(0);

We moved away from the get command, do this instead:

const elem = await $('foo').$$('bar')[0];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review These Pull Requests require review from project members PR: New Feature 🚀 PRs that contain new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants