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

Add initial code for sync to async conversion #53

Merged
merged 16 commits into from
Feb 10, 2022

Conversation

WillBrock
Copy link
Member

@WillBrock WillBrock commented Dec 11, 2021

Expanding off of this pull request

This looks to do the sync to async conversion on most things.

This should convert:

  • Transforms describe, before, it to async
  • Page class method calls, including chained calls
  • Sets all method definitions to async, excludes getters and setters.
  • All built in methods to async
  • By default, this adds awaits to most everything but the small amount of calls that don't need to be awaited can be added to the constants file.

Add to constants arrays for custom items unique to a codebase

In async/constants.js

AWAIT_CUSTOM_GETTERS - Add getters here that will need to be awaited. e.g. things like $$

get foo() { $$(`.foobar`); }

EXCLUDE_METHODS - Add class methods here that don't need to be async / await

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 11, 2021

CLA Signed

The committers are authorized under a signed CLA.

@WillBrock
Copy link
Member Author

For the forEach parts I think it might be easiest to do the following with jscodeshift but I'm not sure how to do it.

From:

[1,2,3].forEach(num => {
     browser.pause(5000);
})

To:

await Promise.all([1,2,3].map(async num => {
    await browser.pause(5000);
});

Any suggestions would be helpful.

@christian-bromann
Copy link
Member

@WillBrock thanks for the work! Have you tried https://astexplorer.net/? It is really great to understand how one piece of code needs to be replaced with the other. Also would you mind extending the test cases so we are sure to not introduce any bugs down the line?

@WillBrock
Copy link
Member Author

Fixed a few things and added converting forEach to regular for loops. Still a work in progress.

Will add some tests as well.

@WillBrock
Copy link
Member Author

I think I've got the majority of things covered. I added tests for it but the tsx parser ones are failing because it doesn't add async to method definitions. I'm not sure why those are failing so something I'll need to read up more on.

@WillBrock
Copy link
Member Author

Still some more edge cases I need to do that are coming up.

@christian-bromann
Copy link
Member

@WillBrock great work! Let me know when this is ready to review and I am happy to take a look.

@kailin0512
Copy link

@WillBrock Great work! is it ready to use or you need some help on that?

@WillBrock
Copy link
Member Author

@kailin0512 It's just about ready. I've just got a few things to clean up and I'll try to push the changes tomorrow.

@WillBrock
Copy link
Member Author

I think this is basically ready. I'm still going through some of our codebase at work (about 3k files) but most things are working.

If anyone wants to try it out feel free and let me know what issues you run into.

@WillBrock
Copy link
Member Author

@christian-bromann This is ready.

@christian-bromann
Copy link
Member

Awesome work @WillBrock , let me review it asap.

@owens-ben
Copy link

@christian-bromann any update on approving this?

@owens-ben
Copy link

and @WillBrock how can I try this out before it's published? I'm happy to help test it.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review. This looks fantastic, thanks @WillBrock 👏

@christian-bromann christian-bromann merged commit 71ef482 into webdriverio:main Feb 10, 2022
@jameskip
Copy link

Thank you @WillBrock for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants