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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Aug 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
297e41b
factor console logging out of run.ts
flotwig Aug 25, 2022
173de9f
fix print-run
flotwig Aug 25, 2022
e493600
minimize diff
flotwig Aug 25, 2022
beb3f2d
chore(server): convert browsers/index to typescript
flotwig Aug 25, 2022
4957a83
fix tests
flotwig Aug 25, 2022
c255548
update stubbed tests
flotwig Aug 25, 2022
b01e8c0
convert electron.js to .ts
flotwig Aug 25, 2022
ce15192
Suggestions from code review
flotwig Aug 26, 2022
8bd49f3
Clean up new type errors
flotwig Aug 26, 2022
8e8f62a
electron.connectToExisting can be sync
flotwig Aug 26, 2022
501895a
more type errors for the type god
flotwig Aug 26, 2022
5e74399
Suggestions from code review
flotwig Aug 26, 2022
ab66d07
Merge branch 'develop' into browser-types
flotwig Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/app/cypress/e2e/cypress-in-cypress-component.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ describe('Cypress In Cypress CT', { viewportWidth: 1500, defaultCommandTimeout:
expect(ctx.actions.browser.setActiveBrowserById).to.have.been.calledWith(browserId)
expect(genId).to.eql('firefox-firefox-stable')
expect(ctx.actions.project.launchProject).to.have.been.calledWith(
ctx.coreData.currentTestingType, {}, o.sinon.match(new RegExp('cypress\-in\-cypress\/src\/TestComponent\.spec\.jsx$')),
ctx.coreData.currentTestingType, undefined, o.sinon.match(new RegExp('cypress\-in\-cypress\/src\/TestComponent\.spec\.jsx$')),
)
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/app/cypress/e2e/top-nav.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('App Top Nav Workflows', () => {
expect(ctx.actions.browser.setActiveBrowserById).to.have.been.calledWith(browserId)
expect(genId).to.eql('edge-chromium-stable')
expect(ctx.actions.project.launchProject).to.have.been.calledWith(
ctx.coreData.currentTestingType, {}, undefined,
ctx.coreData.currentTestingType, undefined, undefined,
)
})
})
Expand Down
8 changes: 4 additions & 4 deletions packages/data-context/src/actions/ProjectActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { CodeGenType, MutationSetProjectPreferencesInGlobalCacheArgs, NexusGenObjects, NexusGenUnions } from '@packages/graphql/src/gen/nxs.gen'
import type { InitializeProjectOptions, FoundBrowser, FoundSpec, LaunchOpts, OpenProjectLaunchOptions, Preferences, TestingType, ReceivedCypressOptions, AddProject, FullConfig, AllowedState, SpecWithRelativeRoot } from '@packages/types'
import type { InitializeProjectOptions, FoundBrowser, FoundSpec, OpenProjectLaunchOptions, Preferences, TestingType, ReceivedCypressOptions, AddProject, FullConfig, AllowedState, SpecWithRelativeRoot, OpenProjectLaunchOpts } from '@packages/types'
import type { EventEmitter } from 'events'
import execa from 'execa'
import path from 'path'
Expand All @@ -22,7 +22,7 @@ export interface ProjectApiShape {
* order for CT to startup
*/
openProjectCreate(args: InitializeProjectOptions, options: OpenProjectLaunchOptions): Promise<unknown>
launchProject(browser: FoundBrowser, spec: Cypress.Spec, options: LaunchOpts): Promise<void>
launchProject(browser: FoundBrowser, spec: Cypress.Spec, options?: OpenProjectLaunchOpts): Promise<void>
insertProjectToCache(projectRoot: string): Promise<void>
removeProjectFromCache(projectRoot: string): Promise<void>
getProjectRootsFromCache(): Promise<ProjectShape[]>
Expand Down Expand Up @@ -175,7 +175,7 @@ export class ProjectActions {
// When switching testing type, the project should be relaunched in the previously selected browser
if (this.ctx.coreData.app.relaunchBrowser) {
this.ctx.project.setRelaunchBrowser(false)
await this.ctx.actions.project.launchProject(this.ctx.coreData.currentTestingType, {})
await this.ctx.actions.project.launchProject(this.ctx.coreData.currentTestingType)
}
})
} catch (e) {
Expand Down Expand Up @@ -228,7 +228,7 @@ export class ProjectActions {
}
}

async launchProject (testingType: Cypress.TestingType | null, options: LaunchOpts, specPath?: string | null) {
async launchProject (testingType: Cypress.TestingType | null, options?: OpenProjectLaunchOpts, specPath?: string | null) {
if (!this.ctx.currentProject) {
return null
}
Expand Down
2 changes: 1 addition & 1 deletion packages/data-context/src/data/ProjectLifecycleManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ export class ProjectLifecycleManager {
if (this.ctx.coreData.activeBrowser) {
// if `cypress open` was launched with a `--project` and `--testingType`, go ahead and launch the `--browser`
if (this.ctx.modeOptions.project && this.ctx.modeOptions.testingType) {
await this.ctx.actions.project.launchProject(this.ctx.coreData.currentTestingType, {})
await this.ctx.actions.project.launchProject(this.ctx.coreData.currentTestingType)
}

return
Expand Down
1 change: 1 addition & 0 deletions packages/frontend-shared/cypress/e2e/support/e2eSupport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@flotwig flotwig Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)


if (!o.skipMockingPrompts
Expand Down
2 changes: 1 addition & 1 deletion packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ export const mutation = mutationType({
specPath: stringArg(),
},
resolve: async (_, args, ctx) => {
await ctx.actions.project.launchProject(ctx.coreData.currentTestingType, {}, args.specPath)
await ctx.actions.project.launchProject(ctx.coreData.currentTestingType, undefined, args.specPath)

return ctx.lifecycleManager
},
Expand Down
2 changes: 1 addition & 1 deletion packages/server/lib/browsers/cdp_automation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export const normalizeResourceType = (resourceType: string | undefined): Resourc
}

type SendDebuggerCommand = (message: string, data?: any) => Promise<any>
type SendCloseCommand = (shouldKeepTabOpen: boolean) => Promise<any>
type SendCloseCommand = (shouldKeepTabOpen: boolean) => Promise<any> | void
type OnFn = (eventName: string, cb: Function) => void

// the intersection of what's valid in CDP and what's valid in FFCDP
Expand Down
79 changes: 41 additions & 38 deletions packages/server/lib/browsers/chrome.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ import { BrowserCriClient } from './browser-cri-client'
import type { LaunchedBrowser } from '@packages/launcher/lib/browsers'
import type { CRIWrapper } from './cri-client'
import type { Automation } from '../automation'

// TODO: this is defined in `cypress-npm-api` but there is currently no way to get there
type CypressConfiguration = any
import type { BrowserLaunchOpts, BrowserNewTabOpts } from '@packages/types'

const debug = debugModule('cypress:server:browsers:chrome')

Expand Down Expand Up @@ -123,7 +121,7 @@ const DEFAULT_ARGS = [
'--disable-dev-shm-usage',
]

let browserCriClient
let browserCriClient: BrowserCriClient | undefined

/**
* Reads all known preference files (CHROME_PREFERENCE_PATHS) from disk and retur
Expand Down Expand Up @@ -433,8 +431,8 @@ const _handlePausedRequests = async (client) => {
})
}

const _setAutomation = async (client: CRIWrapper.Client, automation: Automation, resetBrowserTargets: (shouldKeepTabOpen: boolean) => Promise<void>, options: CypressConfiguration = {}) => {
const cdpAutomation = await CdpAutomation.create(client.send, client.on, resetBrowserTargets, automation, options.experimentalSessionAndOrigin)
const _setAutomation = async (client: CRIWrapper.Client, automation: Automation, resetBrowserTargets: (shouldKeepTabOpen: boolean) => Promise<void>, options: BrowserLaunchOpts) => {
const cdpAutomation = await CdpAutomation.create(client.send, client.on, resetBrowserTargets, automation, !!options.experimentalSessionAndOrigin)

return automation.use(cdpAutomation)
}
Expand Down Expand Up @@ -490,7 +488,7 @@ export = {
return extensionDest
},

_getArgs (browser: Browser, options: CypressConfiguration, port: string) {
_getArgs (browser: Browser, options: BrowserLaunchOpts, port: string) {
const args = ([] as string[]).concat(DEFAULT_ARGS)

if (os.platform() === 'linux') {
Expand Down Expand Up @@ -551,43 +549,60 @@ export = {
return args
},

async connectToNewSpec (browser: Browser, options: CypressConfiguration = {}, automation: Automation) {
async connectToNewSpec (browser: Browser, options: BrowserNewTabOpts, automation: Automation) {
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')
Copy link
Contributor

Choose a reason for hiding this comment

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

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'

Copy link
Member

Choose a reason for hiding this comment

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

This should actually close #22451

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


const pageCriClient = browserCriClient.currentlyAttachedTarget

if (!pageCriClient) throw new Error('Missing pageCriClient in connectToNewSpec')

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},

async connectToExisting (browser: Browser, options: BrowserLaunchOpts, automation) {
const port = await protocol.getRemoteDebuggingPort()

debug('connecting to existing chrome instance with url and debugging port', { url: options.url, port })
if (!options.onError) throw new Error('Missing onError in connectToExisting')

const browserCriClient = await BrowserCriClient.create(port, browser.displayName, options.onError, onReconnect)

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

const pageCriClient = await browserCriClient.attachToTargetUrl(options.url)

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

async attachListeners (browser: Browser, url: string, pageCriClient, automation: Automation, options: BrowserLaunchOpts & { onInitializeNewBrowserTab?: () => void }) {
if (!browserCriClient) throw new Error('Missing browserCriClient in attachListeners')

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

// make sure page events are re enabled or else frame tree updates will NOT work as well as other items listening for page events
await pageCriClient.send('Page.enable')

await options.onInitializeNewBrowserTab()
await options.onInitializeNewBrowserTab?.()

await Promise.all([
this._maybeRecordVideo(pageCriClient, options, browser.majorVersion),
this._handleDownloads(pageCriClient, options.downloadsFolder, automation),
])

await this._navigateUsingCRI(pageCriClient, options.url)
await this._navigateUsingCRI(pageCriClient, url)

if (options.experimentalSessionAndOrigin) {
await this._handlePausedRequests(pageCriClient)
_listenForFrameTreeChanges(pageCriClient)
}
},

async connectToExisting (browser: Browser, options: CypressConfiguration = {}, automation) {
const port = await protocol.getRemoteDebuggingPort()

debug('connecting to existing chrome instance with url and debugging port', { url: options.url, port })
const browserCriClient = await BrowserCriClient.create(port, browser.displayName, options.onError, onReconnect)
const pageCriClient = await browserCriClient.attachToTargetUrl(options.url)

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

async open (browser: Browser, url, options: CypressConfiguration = {}, automation: Automation): Promise<LaunchedBrowser> {
async open (browser: Browser, url, options: BrowserLaunchOpts, automation: Automation): Promise<LaunchedBrowser> {
const { isTextTerminal } = options

const userDir = utils.getProfileDir(browser, isTextTerminal)
Expand Down Expand Up @@ -646,6 +661,8 @@ export = {
// SECOND connect to the Chrome remote interface
// and when the connection is ready
// navigate to the actual url
if (!options.onError) throw new Error('Missing onError in chrome#open')

browserCriClient = await BrowserCriClient.create(port, browser.displayName, options.onError, onReconnect)

la(browserCriClient, 'expected Chrome remote interface reference', browserCriClient)
Expand All @@ -669,7 +686,7 @@ export = {
debug('closing remote interface client')

// Do nothing on failure here since we're shutting down anyway
browserCriClient.close().catch()
browserCriClient?.close().catch()
browserCriClient = undefined

debug('closing chrome')
Expand All @@ -679,21 +696,7 @@ export = {

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

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

Choose a reason for hiding this comment

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

Nice deduping of this code here. Good clean up


await pageCriClient.send('Page.enable')

await Promise.all([
this._maybeRecordVideo(pageCriClient, options, browser.majorVersion),
this._handleDownloads(pageCriClient, options.downloadsFolder, automation),
])

await this._navigateUsingCRI(pageCriClient, url)

if (options.experimentalSessionAndOrigin) {
await this._handlePausedRequests(pageCriClient)
_listenForFrameTreeChanges(pageCriClient)
}
await this.attachListeners(browser, url, pageCriClient, automation, options)

// return the launched browser process
// with additional method to close the remote connection
Expand Down
Loading