chore: improve pkg/driver types #21197

sainthkh · 2022-04-26T06:45:49Z

User facing changelog

N/A. It's adding types for internal use.

Additional details

Why was this change necessary? => Remove anys and make code easier to navigate.
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?
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 · 2022-04-26T06:45:52Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

sainthkh · 2022-04-26T07:01:32Z

packages/driver/src/cypress/command.ts

-// mixin lodash methods
-_.each(['pick'], (method) => {
-  return $Command.prototype[method] = function (...args) {
-    args.unshift(this.attributes)
-
-    return _[method].apply(_, args)
-  }
-})
-


This code is added 5 years ago and no other lodash function is added to $Command. I guess it wouldn't later. So, I moved it inside the class for type.

sainthkh · 2022-04-26T07:10:53Z

packages/driver/src/cypress/state.ts

@@ -0,0 +1,35 @@
+/// <reference path="../../types/cy/commands/session.d.ts" />


New file is created because

types defined in .ts cannot be imported to d.ts files without breaking d.ts's auto import feature.

d.ts files automatically changed undefined or misspelled types to any.

jennifer-shehane · 2022-04-27T15:51:41Z

@sainthkh Thanks for the updates!

BlueWinds · 2022-05-04T23:44:31Z

packages/driver/src/cy/assertions.ts

@@ -347,6 +349,8 @@ export const create = (Cypress, cy) => {
        if (_.isFunction(onRetry)) {
          return cy.retry(onRetry, options)
        }
+
+        return


Do we need this trailing empty return? It looks very unusual to my eyes.

It's because TypeScript wants all return paths.

As the type of cy.retry became specific, TypeScript learned that this function returns a Promise above but nothing at the end.

Without this it's an error.

emilyrohrbough · 2022-05-06T16:11:19Z

packages/driver/src/cypress/command.ts

+  pick (...args) {
+    args.unshift(this.attributes)
+
+    return _.pick.apply(_, args as [Record<string, any>, any[]])
+  }


I don't necessarily think we need to be added this to every command instance. Why can't we use a static method?

It's used in the line below:

cypress/packages/driver/src/cypress/log.ts

Line 125 in 331e541

_.defaults(obj, current != null ? current.pick('name', 'type') : undefined)

And because of the this in the this.attributes. You cannot use non-static fields with this inside static functions.

emilyrohrbough

@sainthkh Can you share the long-term intended type changes? I'm having a hard time understanding the end-goal for the driver package.

It seems like our types patterns are getting inconsistent where some components are exporting their types from their file where other types are being managed in a standalone file, while other types are overriding the CLI types within ./driver/types

sainthkh · 2022-05-09T06:43:39Z

AFAIK, there is no rule or documentation about where and how we should define our types. But as a person who wrote a lot of type definitions for Cypress, here are my rules and goals:

Use cli/types when the type should be provided to the users.
Use driver/types when we're extending the provided types (e.g. Window) or it's not defined in our code.
Use each ts file and import/import type syntax when it's defined in our code.

The reasons are below. It's a bit long because it's related with the history of Cypress code base.

Why are we using TypeScript?

Maybe, we should start with the benefit of TypeScript over plain JavaScript. I think there are 2 benefits:

Static types make you think about the correct input and output of everything. This can help reduce bugs and write easier-to-read code.
Thanks to more info, tools can help us navigate and write code faster with intellisense.

The goal of these type PRs is related with the second reason, making Cypress driver code easier to navigate.

Without types, when a developer wants to know where the definition of some functions are, they have to search the entire project and read all the result. With types, they can save much time by just clicking the name.

`cli/types` - public types for users.

driver imports types from 3 sources:

cli/types - d.ts
driver/types - d.ts
each file

cli/types were first created because some users wanted TypeScript support of Cypress while the code base was mostly written in CoffeeScript. It was inevitable because it's the only way to use TypeScript and CoffeeScript together.

There was an issue like #6683 that the types should be generated from the driver directly. But it's not possible in the near future because cy commands are added dynamically to the class $Cy like below:

cypress/packages/driver/src/cypress/commands.ts

Lines 188 to 193 in 331e541

    
           // perf loop 
        
           for (let cmd of builtInCommands) { 
        
             // support "export default" syntax 
        
             cmd = cmd.default || cmd 
        
             cmd(Commands, Cypress, cy, state, config) 
        
           }

When removing type todos in #20601, I thought about moving command options to driver and adding them to the cli/type on the build. But it would make the problem more complicated because public and internal interfaces are mixed in the project. So, I decided not to do that and imported types from cli/types when it's public.

And removing it is a bit hard, because there are some APIs we need to hide from our definition. For example of #20556, $Cy uses eventemitter2 internally, but it should not expose waitFor because it can confuse users.

`driver/types`

The next type file created is driver/types. It is created mainly to avoid type problems in driver when we're moving JavaScript to TypeScript.

But there are some limitation of this file because:

type check isn't stronger than plain .ts files. For example, there is currently ActiveSessionsSessionData in internal-types.d.ts. But this type doesn't exist. It should be Cypress.Commands.Session.ActiveSessions. But TypeScript doesn't alarm because non-existent types are any in d.ts files.
It doesn't help navigating files. When you want to learn more about something, it doesn't lead to the real definition, but the d.ts. It's not what we wanted.

So, whenever possible, I try to define types inside the .ts files, and use import or import type directly from the defined .ts file to help navigate.

But removing these d.ts files isn't the goal, because they are sometimes necessary when we're extending the given types like Window in types/window.d.ts in this PR.

rachelruderman

LGTM, thank you for your contribution and the in-depth explanation in the comments 🙂

sainthkh added 10 commits April 26, 2022 15:30

Add Cypress.State to $Cypress class.

360773f

Add types of Cypress and state.

8e566ac

Add ICookies type.

263e937

Add CypressRunnable.

77ab766

Cypress.State to timeouts.ts

93ea129

stability.ts

9b01136

Remove @ts-ignore.

6f50e36

Use $Command.

96c7a2f

assertions.ts

43df379

Cypress.State to StateFunc.

9385b34

fix

51c6f52

sainthkh commented Apr 26, 2022

View reviewed changes

Add types.

7d513c3

sainthkh commented Apr 26, 2022

View reviewed changes

sainthkh added 4 commits April 26, 2022 16:18

chai.ts

5ef74fa

xhrs.ts timers.ts

8b0758f

aliases.ts

e384cbd

ensures.ts

534e843

sainthkh marked this pull request as ready for review April 26, 2022 07:30

sainthkh requested a review from a team as a code owner April 26, 2022 07:30

sainthkh requested review from jennifer-shehane and removed request for a team April 26, 2022 07:30

jennifer-shehane removed their request for review April 27, 2022 15:51

MuazOthman requested a review from emilyrohrbough April 29, 2022 15:36

Merge branch 'develop' into improve-types

37fa2f6

sainthkh mentioned this pull request May 2, 2022

#21211 broke the build for external contributors. #21284

Closed

Merge branch 'develop' into improve-types

cec2f66

sainthkh mentioned this pull request May 3, 2022

chore: Distribute tests to desktop-gui containers. Make desktop-gui tests faster! #21305

Merged

1 task

flotwig requested a review from BlueWinds May 4, 2022 15:32

BlueWinds approved these changes May 4, 2022

View reviewed changes

Merge branch 'develop' into improve-types

9e2c0dc

emilyrohrbough reviewed May 6, 2022

View reviewed changes

ryanthemanuel requested a review from rachelruderman May 9, 2022 15:36

rachelruderman approved these changes May 11, 2022

View reviewed changes

flotwig merged commit 38bd1bb into cypress-io:develop May 13, 2022

sainthkh mentioned this pull request May 24, 2022

chore: Improve pkg/driver types part 2 #21610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve pkg/driver types #21197

chore: improve pkg/driver types #21197

sainthkh commented Apr 26, 2022

cypress-bot bot commented Apr 26, 2022

sainthkh Apr 26, 2022

sainthkh Apr 26, 2022 •

edited

jennifer-shehane commented Apr 27, 2022

BlueWinds May 4, 2022

sainthkh May 5, 2022

sainthkh May 5, 2022

emilyrohrbough May 6, 2022

sainthkh May 9, 2022 •

edited

emilyrohrbough left a comment

sainthkh commented May 9, 2022

rachelruderman left a comment

		@@ -0,0 +1,35 @@
		/// <reference path="../../types/cy/commands/session.d.ts" />

chore: improve pkg/driver types #21197

chore: improve pkg/driver types #21197

Conversation

sainthkh commented Apr 26, 2022

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 26, 2022

sainthkh Apr 26, 2022

Choose a reason for hiding this comment

sainthkh Apr 26, 2022 • edited

Choose a reason for hiding this comment

jennifer-shehane commented Apr 27, 2022

BlueWinds May 4, 2022

Choose a reason for hiding this comment

sainthkh May 5, 2022

Choose a reason for hiding this comment

sainthkh May 5, 2022

Choose a reason for hiding this comment

emilyrohrbough May 6, 2022

Choose a reason for hiding this comment

sainthkh May 9, 2022 • edited

Choose a reason for hiding this comment

emilyrohrbough left a comment

Choose a reason for hiding this comment

sainthkh commented May 9, 2022

Why are we using TypeScript?

cli/types - public types for users.

driver/types

rachelruderman left a comment

Choose a reason for hiding this comment

sainthkh Apr 26, 2022 •

edited

sainthkh May 9, 2022 •

edited

`cli/types` - public types for users.

`driver/types`