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: Remove pkg/driver @ts-nocheck part 1 #19353

Merged

Conversation

sainthkh
Copy link
Contributor

User facing changelog

N/A. It's just removing // @ts-nocheck comments and defining types to avoid type errors.

Additional details

  • Why was this change necessary? => To make pkg/driver files more type-safe.
  • 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 Dec 14, 2021

Thanks for taking the time to open a PR!

Comment on lines -535 to +548
const skipMouseupEvent = mouseDownPhase.events.pointerdown.skipped || mouseDownPhase.events.pointerdown.preventedDefault
const skipMouseupEvent = mouseDownPhase.events.pointerdown.preventedDefault
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By definition, mouseDownPhase.events.pointerdown.skipped is always undefined.

@sainthkh sainthkh marked this pull request as ready for review December 14, 2021 02:32
@@ -720,6 +740,9 @@ const sendEvent = (evtName, el, evtOptions, bubbles = false, cancelable = false,
evt.stopPropagation = function (...args) {
evt._hasStoppedPropagation = true

// stopPropagation doesn't have argument. So, we cannot type-safely pass the second argument.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// stopPropagation doesn't have argument. So, we cannot type-safely pass the second argument.
// stopPropagation doesn't have any arguments. So, we cannot type-safely pass the second argument.

ryanthemanuel
ryanthemanuel previously approved these changes Dec 15, 2021
mjhenkes
mjhenkes previously approved these changes Dec 15, 2021
@@ -54,12 +52,18 @@ const isDomSubjectAndMatchesValue = (value, subject) => {
return false
}

type Parsed = {
subject?: any
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these subjects always jQuery objects? if that is true, would the type be something like jQuery<TElement>? Not sure it really matters here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I handled it as JQuery<any>. To use JQuery<TElement>, we need the type info of value.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome!

@@ -77,7 +81,7 @@ export const create = (Cypress, cy) => {
const getUpcomingAssertions = () => {
const index = cy.state('index') + 1

const assertions = []
const assertions: any[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const assertions: any[] = []
const assertions: { attributes: EnqueuedCommand}[] = []

I think that is the right type here but I am not sure we have access to it's context in the driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The specific type of any is $Command here. But I made it to any for now. Because it should be worked with $Cy.queue and the other $Cy members.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to have the types be explicit with the the other cy members or address that in another PR? For now any seems like it fits the bill 😄 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Removing // @ts-nocheck from pkg/driver is a big work. So, I'm breaking them down to bite-size.

(Actually, there are sometimes so many things to think. So I'm postponing them for the later PRs.)

packages/driver/src/cy/chai.ts Show resolved Hide resolved
@chrisbreiding chrisbreiding merged commit 01f0261 into cypress-io:develop Dec 20, 2021
tgriesser added a commit that referenced this pull request Dec 21, 2021
…ert-with-stack

* tgriesser/10.0-release/refactor-lifecycle: (50 commits)
  Remove unused test file
  update task spec to use correct projectRoot
  update
  Fix test
  Fix test
  Fix tests
  update tests
  fix test
  correct config path
  Fix TS
  resolve conflicts
  Fixing component & e2e tests
  build: fix dev process on windows (#19401)
  fix: `cy.contains()` ignores `<style>` and `<script>` without removing them. (#19424)
  Fix some tests
  chore: Fix the broken codeowners automation (#19431)
  chore: add types for Cypress.session.clearAllSavedSessions (#19412)
  fix: No unnecessary snapshotting (#19311)
  chore: Remove pkg/driver @ts-nocheck part 1 (#19353)
  fix: add CYPRESS_VERIFY_TIMEOUT param and a test for it (#19282)
  ...
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

5 participants