-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
refactor typedefs for cy #3499
refactor typedefs for cy #3499
Conversation
@bkucera can you add a dtslint test if possible for this? That checks that someone can actually extend |
cli/types/index.d.ts
Outdated
@@ -4237,6 +4237,8 @@ declare namespace Cypress { | |||
// Diff taken from https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-311923766 | |||
type Diff<T extends string, U extends string> = ({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T] | |||
type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>> | |||
|
|||
interface cy extends Chainable<undefined> {} |
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.
can this interface cy
be documented please?
cli/types/tests/plugins-test.ts
Outdated
* Unfortunately we cannot test that vendor types located in node_modules are working | ||
* since those are copied as part of the deploy process | ||
* | ||
*/ |
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.
this needs more documentation - this is what someone who writes a new command or adds a property to cy
global object should do right. So we should describe this better, even for ourselves.
Also, the formatting is all off below, maybe it needs a prettier ...
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.
@bkucera I love the refactoring and the type test, and this seems like the simplest way to extend Cypress with a custom command, but I would love to see more JSDoc comments before merging it
@@ -16,6 +16,7 @@ | |||
"check-deps-pre": "npm run check-deps -- --prescript", | |||
"dtslint": "dtslint types", | |||
"postinstall": "node ./scripts/post-install.js", | |||
"lint": "bin-up eslint --fix *.js scripts/*.js bin/* lib/*.js lib/**/*.js test/*.js test/**/*.js", |
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.
is this the right command now? to lint?
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.
looks good
I have tried this change against |
for users/plugin-authors who just want to extend
cy
and not theCypress.Chainable
for example, I want to do this:
to indicate I can only access my property on the parent
cy
tasks