Reusable browser automation

MichaelBelousov commented 8 months ago

extract playwright automation of login page out of TestBrowserAuthorizationClient and into independent SignInAutomation module in @itwin/oidc-signin-tool. The logic is 99% the same (everything except handling of undefined authorization_endpoint) should be a pretty much exact lift
add loginElectronMainAuthClient function module to in @itwin/oidc-signin-tool, with a high level usage of SignInAutomation for electron apps, including those that run iTwin.js backends in an electron utility process

I have changed this to be an internal API change with no peer dependency changes since they shouldn't be required anymore (although doing so does force a useless playwright+chromium install on consumers). Once we've vetted it a bit with some testers, then we can consider publishing it, probably as a separate package.

ben-polinsky commented 8 months ago

Adding a peer dep is definitely a breaking change, but I don't see a problem with that. Is it really optional though?

with a high level usage of SignInAutomation for electron apps, including those that run iTwin.js backends in an electron utility process

I just want to make sure these changes are intended for testing and not for other automation.

MichaelBelousov commented 8 months ago

Adding a peer dep is definitely a breaking change, but I don't see a problem with that. Is it really optional though?

with a high level usage of SignInAutomation for electron apps, including those that run iTwin.js backends in an electron utility process

I just want to make sure these changes are intended for testing and not for other automation.

yes they are solely for testing. I will think about how to remove the peer dep... it is necessary for the case in which a consumer of the library brings their own playwright. You can't load two instances of playwright into the same node.js process, so then this package couldn't use its own dependency and the package manager has to place things correctly.

I suppose I might be able to do it by dynamically requireing playwright at usage sites throughout this package, that way if you use an API that requires passing your own playwright Page, then this package will not attempt to load its own playwright. But I will need to test consuming this from a consumer both with and without playwright

ben-polinsky commented 8 months ago

yes they are solely for testing

Cool.

Yeah the double playwright issue has been annoying with this tool. I think that adding it as a non-optional peer dep could be okay. I don't mind a breaking change if it clears this up once and for all.

MichaelBelousov commented 7 months ago

@ben-polinsky bump? we just got the flakiness that this should help, again in CI

Also, I was thinking about the peer dependency... it might be possible to install playwright if it can't be found, as a postinstall script, but I'm really not sure.

MichaelBelousov commented 7 months ago

this is becoming more prevalent in our CI builds, @aruniverse @ben-polinsky can you review and maybe advise on what a new package would be named if you want me to extract it to one?

Is it possible in the short term to export this and make it an @internal api? We can preserve the API when we publish it as a separate package.

iTwin / auth-clients

Reusable browser automation #215