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 final #20169

Merged
merged 14 commits into from
Feb 25, 2022

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 Feb 14, 2022

Thanks for taking the time to open a PR!

@sainthkh sainthkh force-pushed the remove-ts-nocheck-final branch 4 times, most recently from 41f282d to 339c30b Compare February 21, 2022 00:47
@sainthkh sainthkh marked this pull request as ready for review February 21, 2022 02:05
@sainthkh sainthkh requested a review from a team as a code owner February 21, 2022 02:05
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team February 21, 2022 02:05
@jennifer-shehane jennifer-shehane removed their request for review February 22, 2022 16:28
flotwig
flotwig previously approved these changes Feb 23, 2022
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

There is a lot of any in here, but this is still quite an improvement, nice work @sainthkh. Looking forward to future PRs that improve type safety even more.

packages/driver/src/cypress.ts Outdated Show resolved Hide resolved

addToLogs(log)
export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be cool to get rid of export default { function1, function2 } and use export function1; export function2 where possible

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 didn't change it because we're currently importing them as import $Log from './log'. I guess this can be fixed after refactoring log to class.

packages/driver/src/cypress/runner.ts Outdated Show resolved Hide resolved
ryanthemanuel
ryanthemanuel previously approved these changes Feb 23, 2022
@@ -449,7 +459,7 @@ const overrideRunnerHook = (Cypress, _runner, getTestById, getTest, setTest, get

const getTestResults = (tests) => {
return _.map(tests, (test) => {
const obj = _.pick(test, 'id', 'duration', 'state')
const obj: any = _.pick(test, 'id', 'duration', 'state')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be Record<string, any>?

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 changed it that way.

@@ -75,7 +187,7 @@ class $Cypress {
this.setConfig(config)
}

setConfig (config = {}) {
setConfig (config: any = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be Record<string, any>?

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 changed it that way.

@@ -86,7 +84,7 @@ const getSnapshotProps = (attrs) => {
return _.pick(attrs, SNAPSHOT_PROPS)
}

const countLogsByTests = function (tests = {}) {
const countLogsByTests = function (tests: any = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be Record<string, any>?

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 changed it that way.

@sainthkh sainthkh dismissed stale reviews from ryanthemanuel and flotwig via dc92a73 February 25, 2022 01:34
@sainthkh sainthkh force-pushed the remove-ts-nocheck-final branch from 24d9069 to dc92a73 Compare February 25, 2022 01:34
@flotwig flotwig merged commit 995798c into cypress-io:develop Feb 25, 2022
tgriesser added a commit that referenced this pull request Mar 2, 2022
* 10.0-release:
  fix: comment link to accurate docs (#20437)
  fix: scaffold commands file (#20398)
  fix(launchpad): support default export (#20383)
  feat(launchpad): support for Vue CLI 5 (#20413)
  fix: UNIFY-676 browsers should be configurable in setupNodeEvents (#20367)
  fix: make launchpad link open default browser (#20399)
  fix(icons): publish the files in the package
  fix: build icons in build-prod (#20411)
  test: migrate module-api to 10.0
  chore: build this branch
  test: migrate module_api to system tests (#20265)
  chore: remove pkg/driver //@ts-nocheck final (#20169)
  chore: fix "cannot find module" in clone-repo-and-checkout-release-branch (#20293)
  chore: Update Chrome (beta) to 99.0.4844.45 (#20234)
  chore: fix CI cache state for darwin (#20339)
  Add TODO comments
  feedback
  chore: move tests to its own file.
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.

3 participants