Add `Sync` to the name to all methods that are not yet async

[mstoykov]

#428 will be moving APIs over.

But it might be better to rename all of those to have Sync at the end.

This way when we add goto with promises you can still use gotoSync and it will work for a release or two or w/e without the script instantly breaking until you rewrite with each instance.

This will let us add the async counterpart and then have both for a while and then later remove the sync one. Without scripts breaking right away without a time for any rewrite where both will work.

[ankur22]

I like this approach. The issue is that we're going to be breaking everyone's script, and so this should probably be done asap (before the next release), just so that we avoid causing issues with our core users.

From what I was hearing in the sync meeting, it seems like this is the simplest approach too, whereas the other method of working with different import modules had a lot of unknowns.

We have to remember to update all docs and example scripts too though.

[mstoykov]

I like this approach. The issue is that we're going to be breaking everyone's script, and so this should probably be done asap (before the next release), just so that we avoid causing issues with our core users.

yes - just looked at ./api/*.go and there is a lot of API ... am I correct that all of it is exposed?

We have to remember to update all docs and example scripts too though.

luckily the examples should now fail the CI :crossed_fingers:

if we agree on this I can do it probably in half a day (as the API turned out to be a lot bigger than the Async issue make it seem) and probably another half a day to update the docs.

[inancgumus]

But it might be better to rename all of those to have Sync at the end.

I wonder what benefits we will get when we add a Sync prefix/postfix to the sync methods. I'd love to hear what is on your mind.

This way when we add goto with promises you can still use gotoSync and it will work for a release or two or w/e without the script instantly breaking until you rewrite with each instance.

Could you explain a little bit more how goto will work?

Thanks!

[mstoykov]

I wonder what benefits we will get when we add a Sync prefix/postfix to the sync methods. I'd love to hear what is on your mind.

From the point after we do it: Users can have scripts that use gotoSync ( I will be using that as the example).

They can have script using it and when we add goto (which will return a promise and be asynchronous) they can decide to move to it at their pace:

1. they update to xk6-browser 0.number.0.
2. See that it is available along 20 other new async functions and decide to upgrade them one at a time over the following month or three.

In the current case once we implement goto with Promises their script stops working (or at least stops working in the way they expect). They at this point need to update all update APIs or not update xk6-browser at all.

This gets worse if (for example) next release with implement 20 of the APIs at the same time - in that case a user using 15 of them will need to do a lot of updating of their scripts to get it working.

Even worse, in the cloud, we don't let users choose which version they run, so when we update 1 of those APIs and then deploy the cloud for the private(or in the future public) beta. Suddenly the script of the user will break as they now use asynchronous API.

This way we can have both APIs live along side for a while - maybe a release, maybe 3 releases. That is up for debate.

We can have them print warnings "you are using the old synchronous API, move to the new one, this will be deprecated in release v0.X.0 of xk6-browser" (or similar).

We can even keep them around forever or at least until we put xk6-browser in k6. (I guess that will be a nice breaking point :shrug: )

The alternative to add Async or at all naming hte methods not the same as in playwright breaks compatibility with playwright. Which seems a lot more important.

Could you explain a little bit more how goto will work?

I am not certain what you mean here exactly? As in how it will be implemented in xk6-browser or how the user will use it?

In xk6-browser it will return a promise and then resolve/reject it when we have went to the page ... and I guess gotten the Response.

let response = page,goto("https://example.com"
// do something will response

will become

page.goto("https://example.com").then((response) => {
 // do something with response
})

when await is available it will be possible to be

let response = await page,goto("https://example.com"
// do something will response

It do see that most examples in playwright are with locator so

// Navigate and wait for element
await page.goto('https://example.com');
await page.locator('text=Example Domain').waitFor();

// Navigate and click element
// Click will auto-wait for the element
await page.goto('https://example.com');
await page.locator('text=Example Domain').click();

before await will be

// Navigate and wait for element
page.goto('https://example.com').then(() => {
  return page.locator('text=Example Domain').waitFor();
}).then(()=> {

// Navigate and click element
// Click will auto-wait for the element
 return  page.goto('https://example.com');
}).then(() => { return page.locator('text=Example Domain').click();
}).then(() => {
  // whatever code here
})

[inancgumus]

I like this idea, and thanks for the examples 🙇

I'll also use the goto example to keep it simple. Please remember that we need to change about two hundred methods and add the Sync postfix.

Benefits

👍 The most important benefits of this idea I can see from your reply are (please correct me if I'm wrong):

• It leaves the choice of using gotoSync or goto to users. So that they can update their xk6-browser versions without fear. That's a nice benefit 👍
• Keeping user script to work locally and on the cloud even if we make a release with Async APIs. For example, if they're using gotoSync, they can keep using it even after we release a new version. They can also prefer to use goto after we implemented goto async. That's another benefit 👍

Questions

❓ The following questions occurred to me while thinking about this approach. I hope we can clarify them together:

1. What will be our strategy for Click and WaitForNavigation methods? These two are Async now, but the rest of the methods in the xk6-browser are Sync.
2. When will we rename all the Sync methods and release them? Do you envision it done in the next release (v0.6.0)?
3. If we transition to Sync methods in the next release, what will happen to the current goto? Will it stop working in v0.6.0?
4. What would happen to current users' scripts? What will they need to change in their scripts to work with v0.6.0?
5. What will happen to users' scripts after we release the async API after a few cycles? Do they need to change their scripts?
6. Will we maintain and do user-support both for Sync and Async methods (Both our Go code and tests)? What kind of problems we may face while doing so? What will be our strategy?

Answers

I'm sure you and the rest of the team can find better answers. Here are mine:

1. Provide ClickSync and WaitForNavigationSync as well, and keep Click, WaitForNavigation async.
2. We can rename all the sync methods in this release. For example, goto will be gotoSync, and the rest of the methods.
3. goto will stop working. Users will need to use gotoSync.
4. They will need to change all of the methods in their scripts and add Sync if they want to use xk6-browser v0.6.0.
5. Yes, but that will depend on users. They will eventually need to change all of the methods in their scripts back to goto. They can do so for Playwright compatibility and also for the stability of their scripts. For stability, because we'll probably be making fixes in this and the upcoming cycles, the fixes will probably better work only for async APIs.
6. Yes. Needs further discussion and planning.

Possible Downsides

The following are my opinions about the downsides of adding the Sync postfix (if I understand you correctly):

• Current users will need to update all their scripts once and add the Sync postfix for each method in their scripts if they want to use xk6-browser v0.6.0.
• Providing both goto and gotoSync can confuse users.
• When we release, some (maybe most) will need to update their scripts to the Async method counterparts. Then, we'll need to explain both options' ins and outs.
• There can be a lot of users who will still depend on the sync API even after we fully support the Async methods. And some (maybe most) of them won't even care to switch to the Async API for too long, even if we give deprecations warnings. Since they will get used to the Sync methods (like gotoSync) in their scripts. And some parts of their scripts will consist of Async methods (like goto, and others). That can be challenging from the perspective of the effort we will spend on these issues.

---

Another Idea

💡 I'd love us to explore one more idea (and this idea can be a total dumb one):

• Keep the current API as it is and make users import from k6/x/browser-sync in v0.6.0. (I believe this aligns with what @andrewslotin suggested in the meeting).
• Work on the Async API in parallel for the next cycles until we finish it. And, don't document the Async API and don't expose it to users until we finish it.
• Prevent users from importing the k6/x/browser until we finish with the Async API.
• Release the Async API in a single release, update the documentation, and let them import from k6/x/browser.
• Gradually start giving deprecation warnings for the k6/x/browser-sync API and drop support after a while.

Note: If changing import paths turns out the be an ineffective approach, I believe we can still work it out. We can keep using the same import path and don't expose the Async APIs to users until we finish. Then release the Async APIs in one release with the same import path that we currently use.

I believe the benefits of this approach can be:

• Less confusion for users. They will only need to update their import paths to k6/x/browser-sync once if they want to upgrade to the new API until the Async API is ready.
• When we're ready with Async API after a few releases, they will only need to update their import paths to k6/x/browser if they want to upgrade to the Async API.

What are the downsides? 🤔

• Users will need to promisify their code to use the new versions of xk6-browser.
• ? Let's find out together.

---

That's all!

Phew, a lot of stuff here. I'm sorry for this :) I can provide some examples if you want me to.

Since this will be a huge undertaking, in my opinion, it's better to discuss the upsides and downsides of every approach. Note that I'm totally OK with renaming the methods to xxxSync as well.

I'd like to hear WDYT 🙇

@grafana/k6browserdev @mstoykov @sniku

[ankur22]

@inancgumus nice detailed write up. In the sync meeting it seemed that renaming the methods was the simpler approach. It felt like there were some unknowns with creating a new module. What are the challenges in implementing the new module solution?

[inancgumus]

@ankur22 Thanks! Yes, the renaming approach is simple to implement first, but it may come with some baggage afterward, as I explained above (see: the Possible Downsides heading).

The modules approach can be made by simply renaming the following to k6/x/browser-sync:

https://github.com/grafana/xk6-browser/blob/63b8cf63c5aa86ffe70c7f66cbcda34e4bbe5974/main.go#L74-L76

As I mentioned in my previous post, there's another idea similar to the modules approach: Instead of publishing multiple modules (or renaming the methods), we wait until we finish all the Async API features and expose them to users in one release. I'd like to discuss the pros and cons of both approaches (and maybe the new ones you'll bring) with you all.

[imiric]

I think we should discuss this issue in depth in the v0.7.0 planning meeting, but I'll (try to briefly) respond to a few of Inanc's points.

Current users will need to update all their scripts once and add the Sync postfix for each method in their scripts if they want to use xk6-browser v0.6.0.

Right. Though that's a minor downside that will only have to be done once, as you say. This rename can be relatively easily automated, and we could provide a script for users to run. It won't be foolproof, but I doubt users have huge codebases of xk6-browser scripts that this would be a major issue.

Providing both goto and gotoSync can confuse users.

We would maintain both versions only for a few versions of the deprecation period. I don't see the confusion with the names, but the usage might be confusing, since they'll have to mix sync and async versions of methods, keep track of which ones have been implemented, check documentation more often, etc. We would document and announce everything with anticipation, and print usage warnings, but the usage will be challenging.

When we release, some (maybe most) will need to update their scripts to the Async method counterparts. Then, we'll need to explain both options' ins and outs.

Sure, but the explanation will be short. The benefit of this approach is that users will have time (at least a couple of releases) to update at their own pace, instead of their scripts breaking after a single release.

There can be a lot of users who will still depend on the sync API even after we fully support the Async methods.

We will gradually remove sync methods after the deprecation period.

And some (maybe most) of them won't even care to switch to the Async API for too long, even if we give deprecations warnings. Since they will get used to the Sync methods (like gotoSync) in their scripts.

That's on them? By the time we remove sync methods, they would have had several months of notices and deprecation warnings to do the changes.

And some parts of their scripts will consist of Async methods (like goto, and others). That can be challenging from the perspective of the effort we will spend on these issues.

Yes, support will possibly be more challenging.

---

As for the downsides to your proposal @inancgumus, I think the biggest one is that it might take many months for the async API to be fully completed. It's currently our highest priority feature, and we still won't be able to work on it exclusively, so progress will be slow. It's also likely that translating some methods to async will not be trivial.

Instead, I think we should prioritize the most common methods like we did for click() and waitForNavigation(), and release those first, so that users can take advantage of them, since their usage will likely fix many of the race conditions we've seen or will see. Not releasing them as soon as possible will force us to implement fixes or workarounds to race conditions internally, which we'll later have to remove or which might cause issues of their own, instead of having the JS API help us in that sense.

@mstoykov proposed an alternative where we return "fake" Promises in all methods, which could be done in a single release, and then gradually implement async functionality properly. This would ensure we only break the API once, but, as we discussed on Slack, I think this would confuse users expecting methods to actually behave asynchronously, and we would potentially have much more support questions and give confusing explanations of why this is so.

OTOH, waiting several months to release the async API might give us enough time for async/await to be added to goja. :crossed_fingers: This could mean that users would only have to rewrite their scripts once, instead of once for the sync->.then() change, and then for .then()->await change. The second rewrite would be optional, but still highly preferable due to the improved syntax.

---

Let's keep in mind what this issue is about: to give users time to upgrade their scripts once we release new async APIs. It would still be relatively challenging and isn't perfect, but it's the simplest solution if we want to release async support gradually.

The proposal of not releasing async APIs gradually, but all at once, warrants a separate discussion, but I'm slightly against it, for the reasons mentioned above. Another reason I just thought of is: how will the dual module system work exactly? You say we will prevent users from importing k6/x/browser, but then how will we work on it and test it from JS? If there's a dev switch to enable it, what would prevent users from using it? Or will we work on a very long-lived feature branch instead? Neither approach seems appealing.

And the 2 module system presents complex reusability issues that we likely won't realize until we start working on a PoC (as we discussed in the meeting). We'd likely want all async methods to wrap their sync counterparts, to avoid having separate implementations, but that presents some organizational challenges. We would surely need a PoC to decide if this is feasible, but it sounds much more complex than just renaming a method and having its async variant next to it.

[mstoykov]

Let's backtrack a little bit, because I feel like there is some confusion/misunderstanding on what we want the API to look like.

We want to copy the playwright API and their examples to just work.

AFAIK we want to have the exact same API as playwright. Or more accurately we want for as many scripts that work on playwright for them to just work with xk6-browser.

This is why the API is copied from there and has the exact same names.

But a lot of the current methods are synchronous in xk6-browser, but asynchronous in playwright.

Which means that the "just work" won't work.

So in the end we want to change our API to behave the same as playwright.

Am I correct in that? Or does anybody has a different opinion/vision on the matter? (and yes we likely will have incompatibilities and we will extend the API where that seems useful)

The way this is currently done.

As I explained above this is currently done by choosing an API call that is synchronous in xk6-browser , but asynchronous in playwright and then changing it.

This makes us closer to the playwright API, but breaks any xk6-browser script by anyone that used our synchronous version.

This is fine if we moved to asynchronous API in like 1 release or something like that. But not so much given that the current course seems to me like it will take a couple if not more releases.

This is even worse as we intend on having xk6-browser in private/public beta soon :tm: - before we move the whole API. This means that the cloud beta testers won't even have a choice in to revert to an old version to run their tests as the cloud just doesn't support choosing your version.

So this way will continuously on each release - break the scripts for each user up until we move the whole API. And will break it for each cloud beta tester each time we release to the cloud - which will be multiple times per xk6-browser release.

This proposal:

This proposal moves this to two breaking points in time:

1. When we rename all the synchronous API that are async in playwright (this issue)
2. When we finally drop the synchronous API (likely a release + after the last async API is added) - arguably this breaking point can be dropped. But to be honest I don't see why.

Arguably it is better to do 1 as soon as possible - this release. As we would only expect more people to start writing scripts, so any breaking change later is worse then earlier.

2 While will happen later will let users actually move - which IMO will be less of a problem.

Having two import paths - the problems:

The problem here is that the API is on the browser we instantiate, that is imported from the module, not on the module itself. So if we decide that we have one module exposing only async API and the other only sync APIs. I would argue you currently can't do anything with that.

With only the async you get only what we currently have implemented - which is too small to write a test IMO.

With only the synchronous you are missing the async APIs (which are still required) or if you add sync APIs for those - you get the race condition that is fixed by moving to them.

Doing it all in one go

I guess one of the alternatives is to do the whole move to asynchronous APIs in one go. So only breaking the xk6-browser scripts then.

Unfortunately I don't see how we will be able to get this fast enough and it will mean that we break every script likely at least 2 release from now.

It also will be one epic change which IME is pretty bad idea ;)

Comments on the above comments

We would maintain both versions only for a few versions of the deprecation period. I don't see the confusion with the names, but the usage might be confusing, since they'll have to mix sync and async versions of methods, keep track of which ones have been implemented, check documentation more often, etc. We would document and announce everything with anticipation, and print usage warnings, but the usage will be challenging.

I would argue the current case is more confusing - you still mix both, but some of them are the same as playwright, some are named the same but are synchronous.

I would argue that in general the users will be using as many async ones as we have and then fallback to the sync ones.

I think we should prioritize the most common methods like

since their usage will likely fix many of the race conditions we've seen or will see

This IMO means that we might be better off making some additional issues to make async specific APIs that keep seeing breaking. We can test with one and if that makes the test stop failing ... I guess that means that we should just prioritize the ones that fail tests?

[imiric]

After the discussion at last week's sync, we decided to not do this, and will break APIs gradually on each release.