-
Notifications
You must be signed in to change notification settings - Fork 3.4k
chore: remove pkg/driver //@ts-nocheck part 2 #19483
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 2 #19483
Conversation
Thanks for taking the time to open a PR!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other TS newbies curious what this !
is doing: Non-null assertion operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding an explicit type for options if we are reading properties off of it. Or is this represented in the types already?
Edit: Based on your other changes, might need a TODO here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The past tense of cast is cast. My lint spellchecker doesn't flag 'casted' as an unknown word, though 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this answer, casted was used in Middle English and can be used when it's used for cast in programming context.
But I changed all casted
to cast
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I get this done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@finda841 In this PR, there's nothing you can do because I've finished all the suggested changes. If you want to help develop Cypress, you can find ready-for-work
issues here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a few questions and suggestions but otherwise looks like some solid progress!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: change `any` to Loggable | |
// TODO: change the options type from `any` to `Loggable`. |
I'm wondering if for future PRs like this, it might be good to target them at the 10.0-release branch rather than develop. I know the component testing team has added some additional types to cypress, and as we approach releasing that branch, anything we can do to lessen the burden of merging develop -> 10.0 will make their lives easier. Don't need to rebase / retarget these changes, just something to think about for future PRs. |
af1f701
to
152e6df
Compare
152e6df
to
0043826
Compare
create
into class$Cy
#18715User facing changelog
N/A. It's just removing
// @ts-nocheck
comments and defining types to avoid type errors.Additional details
How has the user experience changed?
N/A
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?