chore(server): convert remaining browsers code to typescript #23556

flotwig · 2022-08-25T21:36:18Z

Closes

User facing changelog

Additional details

This PR converts packages/server/lib/browsers/index.js to .ts and adds types for the BrowserLauncher implementations
Also converts electron.js to .ts, although I did not spend as much time adding types here
Converted Bluebird usage to native async/await as convenient
With these changes, everything in server/lib/browsers is finally TSified
Depends on refactor: move console logging out of run.ts #23555

Steps to test

How has the user experience changed?

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-08-25T21:36:20Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-08-25T21:47:19Z

Test summary

39641 • 0 • 3371 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`ab66d07`
Started	Aug 29, 2022 7:14 PM
Ended	Aug 29, 2022 7:28 PM
Duration	14:31 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lmiller1990

Cool, glad to see more TS, I hope we can get more and more strict. In general

seems some un-necesary async usage
consider assert over throw new Error (more concise and seems more idiomatic in my experience)

💯

lmiller1990 · 2022-08-26T00:40:32Z

packages/frontend-shared/cypress/e2e/support/e2eSupport.ts

@@ -294,6 +294,7 @@ function startAppServer (mode: 'component' | 'e2e' = 'e2e', options: { skipMocki
        if (!ctx.lifecycleManager.browsers?.length) throw new Error('No browsers available in startAppServer')

        await ctx.actions.browser.setActiveBrowser(ctx.lifecycleManager.browsers[0])
+        // @ts-expect-error this interface is strict about the options it expects
        await ctx.actions.project.launchProject(o.mode, { url: o.url })


It looks like the interface is

export interface LaunchOpts { browser?: FoundBrowser url?: string automationMiddleware?: AutomationMiddleware projectRoot?: string shouldLaunchNewTab?: boolean onBrowserClose?: (...args: unknown[]) => void onBrowserOpen?: (...args: unknown[]) => void onError?: }

I wonder why we need the ts-expect-error? What is the error?

It's only OpenProjectLaunchOpts at this point, which doesn't have url. This is an area that should be refactored, we're putting a lot of functionality into project.launchProject that makes it difficult to reason with (and difficult to sanely type)

lmiller1990 · 2022-08-26T00:42:29Z

packages/server/lib/browsers/chrome.ts

    debug('connecting to new chrome tab in existing instance with url and debugging port', { url: options.url })

    const browserCriClient = this._getBrowserCriClient()
+
+    if (!browserCriClient) throw new Error('Missing browserCriClient in connectToNewSpec')


I think assert is more common for this kind of thing, eg

cypress/packages/data-context/src/sources/FileDataSource.ts

Line 16 in 937d4e7

assert(this.ctx.currentProject, `Cannot checkIfFileExists without active project`)

assert(browserCriClient, 'Missing browserCriClient in connectToNewSpec')

assert doesn't seem to provide the type guarding needed, which is why I added this conditional. With assert, I still get browserCriClient is possibly 'undefined'

This should actually close #22451

Yeah, at least it will give a clearer error message.

Well...it doesn't fix it but it just give a better warning. I remember talking with @ryanthemanuel about this

we close the browser on a retry:
https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/modes/run.js#L1127
but likely don't flip the indicator that tells it to try and launch the browser from scratch

lmiller1990 · 2022-08-26T00:44:33Z

packages/server/lib/browsers/chrome.ts

+
+    if (!options.url) throw new Error('Missing url in connectToNewSpec')
+
+    await this.attachListeners(browser, options.url, pageCriClient, automation, options)


Instead of marking this as asyncand doingawait this.attachListenershould we just removeasyncand thenreturn this.attachListeners? It looks like we do awaitwhere we call this function so marking it asasyncand then callingawait` at the end doesn't seem to do anything.

Readability-wise, I prefer awaiting a Promise over returning it, because it makes it obvious you're doing an asynchronous operation at the "end" of the function.

packages/server/lib/browsers/electron.ts

lmiller1990 · 2022-08-26T01:02:00Z

packages/server/lib/browsers/electron.ts

-      options = Windows.defaults(options)
+    // get our electron default options
+    // TODO: this is bad, don't mutate the options object
+    options = this._defaultOptions(projectRoot, state, options, automation)


I agree mutation is the main thing that makes server so damn hard to work in :(

packages/server/lib/browsers/electron.ts

lmiller1990 · 2022-08-26T01:06:25Z

packages/server/lib/browsers/index.ts


  if (browser.name === 'electron') {
-    return require('./electron')
+    return require('./electron') as typeof import('./electron')


Not sure if this breaks things but should be able to do something like (I think)

return await import('./electron')

And get typing without as typeof. Also good to standardize on one syntax. Then getBrowserLauncher will return a Promise and well need to be awaited, though.

Or, we can just do

import chrome from './chrome'

Etc all at the top - not sure if there's any performance implication here, but moving towards proper ESM seems like a good idea, so maybe one day we can ship native ESM instead of CommonJS (although this is likely many years away, it would still be nice to work towards it).

Since each browser requires different dependencies, it's done this way for performance. Whether or not it makes a big (or indeed, positive) impact... I'm not sure, but I'm not going to refactor the lazy-loading in this PR. I did switch to await import: ce15192

lmiller1990 · 2022-08-26T01:08:49Z

packages/server/lib/browsers/types.ts

+  isProcessExit?: boolean
+}
+
+export type BrowserLauncher = {


Yay, standardize browser launching interface, nice,

packages/server/lib/browsers/webkit.ts

ryanthemanuel · 2022-08-26T13:44:59Z

packages/server/lib/browsers/electron.ts

  Windows.removeAllExtensions(win)

-  return Bluebird.map(extensionPaths, (extensionPath) => {
+  return Promise.all(extensionPaths.map((extensionPath) => {


ryanthemanuel · 2022-08-26T16:20:55Z

packages/server/lib/browsers/chrome.ts

@@ -679,21 +696,7 @@ export = {

    const pageCriClient = await browserCriClient.attachToTargetUrl('about:blank')

-    await this._setAutomation(pageCriClient, automation, browserCriClient.resetBrowserTargets, options)


Nice deduping of this code here. Good clean up

ryanthemanuel

Nice work. Lots of good clean up here. The only thing I might question (though we do it a lot of places) with the new additional throw statements, do they fail gracefully or crash the server completely?

flotwig · 2022-08-26T17:40:25Z

with the new additional throw statements, do they fail gracefully or crash the server completely?

@ryanthemanuel All the new throws are there for type guarding, where the type isn't providing a sufficient guarantee that a required property is defined in these places. Since our tests don't fail after these changes, I'm thinking it's unlikely users will see these errors. Server error handling should capture these as gracefully as any other browser launching error.

packages/server/lib/browsers/index.ts

packages/server/lib/open_project.ts

flotwig added 6 commits August 25, 2022 17:10

factor console logging out of run.ts

297e41b

fix print-run

173de9f

minimize diff

e493600

chore(server): convert browsers/index to typescript

beb3f2d

fix tests

4957a83

update stubbed tests

c255548

flotwig changed the title ~~chore(server): convert browsers/index to typescript~~ chore: convert browsers/index to typescript Aug 25, 2022

flotwig requested review from ryanthemanuel and marktnoonan August 25, 2022 21:52

flotwig changed the title ~~chore: convert browsers/index to typescript~~ chore(server): convert remaining browsers code to typescript Aug 25, 2022

convert electron.js to .ts

b01e8c0

flotwig requested a review from lmiller1990 August 25, 2022 22:43

flotwig marked this pull request as ready for review August 25, 2022 23:03

lmiller1990 reviewed Aug 26, 2022

View reviewed changes

ryanthemanuel reviewed Aug 26, 2022

View reviewed changes

ryanthemanuel approved these changes Aug 26, 2022

View reviewed changes

flotwig added 2 commits August 26, 2022 13:24

Suggestions from code review

ce15192

Clean up new type errors

8bd49f3

flotwig requested a review from lmiller1990 August 26, 2022 17:36

electron.connectToExisting can be sync

8e8f62a

more type errors for the type god

501895a

emilyrohrbough reviewed Aug 26, 2022

View reviewed changes

packages/server/lib/browsers/index.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Aug 26, 2022

View reviewed changes

packages/server/lib/open_project.ts Show resolved Hide resolved

Suggestions from code review

5e74399

flotwig force-pushed the browser-types branch from 3435d8f to 5e74399 Compare August 26, 2022 21:02

flotwig requested a review from emilyrohrbough August 26, 2022 21:02

lmiller1990 approved these changes Aug 26, 2022

View reviewed changes

flotwig mentioned this pull request Aug 29, 2022

refactor: move more of video capture into browser automations #23587

Merged

2 tasks

Base automatically changed from extract-logging-from-run to develop August 29, 2022 19:05

Merge branch 'develop' into browser-types

ab66d07

flotwig merged commit 3c2fea2 into develop Aug 29, 2022

flotwig deleted the browser-types branch August 29, 2022 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(server): convert remaining browsers code to typescript #23556

chore(server): convert remaining browsers code to typescript #23556

flotwig commented Aug 25, 2022 •

edited

Loading

cypress-bot bot commented Aug 25, 2022

cypress bot commented Aug 25, 2022 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Aug 26, 2022

flotwig Aug 26, 2022 •

edited

Loading

lmiller1990 Aug 26, 2022

flotwig Aug 26, 2022

emilyrohrbough Aug 26, 2022

flotwig Aug 26, 2022

emilyrohrbough Aug 26, 2022

lmiller1990 Aug 26, 2022

flotwig Aug 26, 2022

lmiller1990 Aug 26, 2022

lmiller1990 Aug 26, 2022

flotwig Aug 26, 2022

lmiller1990 Aug 26, 2022

ryanthemanuel Aug 26, 2022

ryanthemanuel Aug 26, 2022

ryanthemanuel left a comment •

edited

Loading

flotwig commented Aug 26, 2022


		if (!options.url) throw new Error('Missing url in connectToNewSpec')

		await this.attachListeners(browser, options.url, pageCriClient, automation, options)

		@@ -679,21 +696,7 @@ export = {

		const pageCriClient = await browserCriClient.attachToTargetUrl('about:blank')

		await this._setAutomation(pageCriClient, automation, browserCriClient.resetBrowserTargets, options)

chore(server): convert remaining browsers code to typescript #23556

chore(server): convert remaining browsers code to typescript #23556

Conversation

flotwig commented Aug 25, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 25, 2022

cypress bot commented Aug 25, 2022 • edited Loading

Test summary

Run details

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanthemanuel left a comment • edited Loading

Choose a reason for hiding this comment

flotwig commented Aug 26, 2022

flotwig commented Aug 25, 2022 •

edited

Loading

cypress bot commented Aug 25, 2022 •

edited

Loading

flotwig Aug 26, 2022 •

edited

Loading

ryanthemanuel left a comment •

edited

Loading