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

Add Cypress.dom.* to TS type declarations #5298

Merged
merged 13 commits into from
Mar 2, 2020

Conversation

kylerob
Copy link
Contributor

@kylerob kylerob commented Oct 5, 2019

Closes #4408

User facing changelog

Cypress.dom.isDetached no longer throws a type error in TypeScript projects.

Additional details

We may want to include all Cypress.dom.* functions in the index.d.ts. If so, I can add the rest of them in this PR.

PR Tasks

  • Have tests been added/updated? (Not sure which test file covers this)
  • Have API changes been updated in the type definitions?

@flotwig
Copy link
Contributor

flotwig commented Oct 7, 2019

@kylerob This PR looks good, but it would be helpful to have the rest of the dom methods as well if you want to go ahead and add them.

@kylerob
Copy link
Contributor Author

kylerob commented Oct 11, 2019

@flotwig I just pushed up the rest of the declarations. Some of the functions were tougher to nail down types for, so I'm not 100% confident in all my declarations. Is there a good automated way to test these before we publish them?

@flotwig flotwig changed the title Add Cypress.dom.isDetached to TS type declaration Add Cypress.dom.* to TS type declarations Oct 11, 2019
@flotwig
Copy link
Contributor

flotwig commented Oct 11, 2019

@flotwig I just pushed up the rest of the declarations. Some of the functions were tougher to nail down types for, so I'm not 100% confident in all my declarations. Is there a good automated way to test these before we publish them?

The easiest way to test these definitions is probably to exercise them in actual Typescript. There are already tests for Cypress.dom here:

https://github.com/cypress-io/cypress/tree/develop/packages/driver/test/cypress/integration/dom

You could rename these .js files to .ts, ensure that the type for Cypress.dom is inferred correctly, and ensure that the types that you've created don't cause any type errors in the tests. You should commit it too so that we can have the type tests going forward.

@jennifer-shehane
Copy link
Member

Hey @kylerob, did you have time to add tests for these changes? If you no longer have time, please close the PR.

@jennifer-shehane
Copy link
Member

@kylerob Unfortunately we have to close this PR due to inactivity.

@rovshenn
Copy link

Any plans to continue on this? Seem that PR was almost there :)

@jennifer-shehane
Copy link
Member

@rovshenn You are welcome to open a new PR to continue the work. We can't maintain stale PRs that have had no commits in 4 months.

@flotwig
Copy link
Contributor

flotwig commented Mar 2, 2020

I can get this one done, it just needs some tests.

@flotwig flotwig reopened this Mar 2, 2020
@flotwig flotwig self-assigned this Mar 2, 2020
@flotwig flotwig requested a review from kuceb March 2, 2020 16:30
Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

@flotwig the tests for typedefs should go in cli/types/tests. can you add tests to cli/types/tests/cypress-tests.ts ?

@flotwig
Copy link
Contributor

flotwig commented Mar 2, 2020

@flotwig the tests for typedefs should go in cli/types/tests. can you add tests to cli/types/tests/cypress-tests.ts ?

sure, i figured converting the existing tests to TS would be enough coverage though

@flotwig flotwig requested a review from kuceb March 2, 2020 17:02
Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

I don't think we'll enjoy keeping this up to date, and it's not in our docs but I think it can go in

@kuceb
Copy link
Contributor

kuceb commented Mar 2, 2020

an alternative would be making Cypress.dom an any type, which would fix the type errors

@flotwig flotwig merged commit 855657d into cypress-io:develop Mar 2, 2020
@rovshenn
Copy link

rovshenn commented Mar 3, 2020

Yes @bkucera , but that reduces the value of the typing :) it is a trade off. Thank you very much @flotwig ! Hopefully I will get my hands dirty with cypress one day

@rovshenn
Copy link

rovshenn commented Mar 3, 2020

@jennifer-shehane could you please tag which release this will be in? Thank you very much for all your assistance!

@jennifer-shehane
Copy link
Member

This will go out in 4.1.1 or 4.2.0 depending on whether there are other features that make it in the next release. Either way - that release is scheduled for Mar 13 at the latest.

@rovshenn
Copy link

rovshenn commented Mar 4, 2020

Thank you very much @jennifer-shehane , I am looking forward to that!

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.

Cypress.dom.isDetached missing from Typescript types
5 participants