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

chore: Improve pkg/driver types part 2 #21610

Merged
merged 17 commits into from Jun 28, 2022

Conversation

sainthkh
Copy link
Contributor

User facing changelog

N/A. It's for internal use.

Additional details

  • Why was this change necessary? => Define types to help navigate files and understand codes.
  • What is affected by this change? => N/A
  • Any implementation details to explain? => N/A

How has the user experience changed?

N/A

PR Tasks

  • [na] 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?
  • [na] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 24, 2022

Thanks for taking the time to open a PR!

Comment on lines -26 to -36
export interface KeyboardState {
keyboardModifiers?: KeyboardModifiers
}

export interface ProxyState<T> {
<K extends keyof T>(arg: K): T[K] | undefined
<K extends keyof T>(arg: K, arg2: T[K] | null): void
}

export type State = ProxyState<KeyboardState>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is added before StateFunc type is created. Now, it's OK to remove them.

Comment on lines -172 to 167
type modifierKeyDetails = KeyDetails & {
type KeyModifiers = {
key: keyof typeof keyToModifierMap
}

const isModifier = (details: KeyInfo): details is modifierKeyDetails => {
const isModifier = (details: KeyInfo): details is KeyDetails & KeyModifiers => {
return details.type === 'key' && !!keyToModifierMap[details.key]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

modifierKeyDetails doesn't follow the TS naming conventions (naming types with the capital letter.) And KeyDetails & cause errors in other files.

// @ts-ignore
import { registerFetch } from 'unfetch'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

//@ts-ignore is unnecessary because the type of registerFetch is added with patch-package.

Comment on lines -241 to -243
// also reset recentlyReady back to null
this.state('recentlyReady', null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Cypress 2017, recentlyReady was necessary to make isReady function (00920b1). This function was removed when refactoring (d4e5b8b).

But somehow this state survived. Removed it.

Comment on lines +125 to +133
interface ICyFocused extends Omit<
IFocused,
'documentHasFocus' | 'interceptFocus' | 'interceptBlur'
> {}

interface ICySnapshots extends Omit<
ISnapshots,
'onCssModified' | 'onBeforeWindowLoad'
> {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I omit those APIs when defining interfaces because they're not $Cy members. But I learned that they are used in cy folder files.

So, I removed omit in the definitions and created new definitions here.

@sainthkh sainthkh changed the title chore: Improve pkg/driver types chore: Improve pkg/driver types part 2 May 24, 2022
const num = _.filter(calls, (call) => _.isEqual(call.args, [50, true, 'dblclick']))
const num = _.filter(calls, (call) => _.isEqual(call.args, [50, true]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cy.timeout has 2 args, not 3.

@sainthkh sainthkh marked this pull request as ready for review May 24, 2022 07:31
@sainthkh sainthkh requested a review from a team as a code owner May 24, 2022 07:31
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team May 24, 2022 07:31
@jennifer-shehane jennifer-shehane removed their request for review June 6, 2022 21:26
@BlueWinds
Copy link
Contributor

@sainthkh - Sorry it's taken so long to get this in, and thanks for your patience. As you may have noticed, we've been having some flake issues recently. It's still on my list here, not forgotten.

@@ -128,7 +131,7 @@ export default (Commands, Cypress, cy, state, config) => {

// add this delay delta to the runnables timeout because we delay
// by it below before performing each click
cy.timeout($actionability.delay, true, eventName)
cy.timeout($actionability.delay, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice to clean this up.

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

Successfully merging this pull request may close these issues.

None yet

4 participants