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: Add dtslint and Cypress static types #1044

Merged

Conversation

NicholasBoll
Copy link
Contributor

@NicholasBoll NicholasBoll commented Dec 8, 2017

This is a break-apart of more completed work. Info can be found at #1040

This particular PR closes #1046

Included in this PR:

  • Add https://github.com/Microsoft/dtslint for type tests
  • Move types into a folder called types per dtslint recommendations (there are a few types files)
  • Add type definitions for Cypress aliased objects like Cypress._, Cypress.$, etc.
  • Add dtslint to unit test task. Not sure the most appropriate spot

I didn't actually add any type tests as all my tests are verifying the completed type definitions.

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2017

CLA assistant check
All committers have signed the CLA.

"dependencies": {
"@cypress/listr-verbose-renderer": "0.4.1",
"@cypress/xvfb": "1.0.4",
"@types/blob-util": "1.3.3",
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'm pretty sure these have to be normal dependencies for downstream projects. I haven't actually deployed and linked as devDependencies, but from other testing the types will fail and fallback to any if the types can't be found.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think they will have to be included then.

*/
requestTimeout: number;
/**
* Time, in milliseconds, to wait until a response in a [cy.request()](https://on.cypress.io/api/request), [cy.wait()](https://on.cypress.io/api/wait), [cy.fixture()](https://on.cypress.io/api/fixture), [cy.getCookie()](https://on.cypress.io/api/getcookie), [cy.getCookies()](https://on.cypress.io/api/getcookies), [cy.setCookie()](https://on.cypress.io/api/setcookie), [cy.clearCookie()](https://on.cypress.io/api/clearcookie), [cy.clearCookies()](https://on.cypress.io/api/clearcookies), and [cy.screenshot()](https://on.cypress.io/api/screenshot) commands
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't have to use https://on.cypress.io/api/request just https://on.cypress.io/request would be better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

There are a lot of other URLs with the extra /api in the URL. I can update those URLs as well.

@@ -910,6 +927,165 @@ declare namespace Cypress {
cancable?: boolean;
}

interface ConfigOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

so we allow all options to be optional using Partial<ConfigOptions>? that is pretty slick if true!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Partial is a mapped type that iterates over the keys and makes each optional. It is really useful if you want an interface without optional parameters and make them optional for things like default overrides.

Mapped types are pretty cool. TypeScript lib.d.ts defines a few globally: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.d.ts#L1329

@bahmutov
Copy link
Contributor

bahmutov commented Dec 8, 2017

@NicholasBoll do you see the broken build in https://circleci.com/gh/cypress-io/cypress/7682 ? since you moved cli/index.d.ts which we included in the built cypress package - do we have to copy cli/types to cli/build/types during npm run build in cli folder as well? Because we are building "production" cypress NPM package

cd cli
npm run build

Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

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

I loved the smart definitions, you are much better at TypeScript than I am!

Small requests to change things, mostly to make sure the build passes through.

@NicholasBoll
Copy link
Contributor Author

I'll fix the build issue. I'll use #979 to make sure get all the path changes corrected. Types aren't required to go into a types folder, but I think it helps reduce the clutter in the cli directory since there are supporting files and type tests.

@NicholasBoll
Copy link
Contributor Author

I'll also make sure the package contains all the type definitions using npm pack.

@NicholasBoll
Copy link
Contributor Author

I've noticed that JS files don't have semicolons. I think the TSLint and type definitions shouldn't have them either.

@bahmutov
Copy link
Contributor

bahmutov commented Dec 8, 2017

all great stuff @NicholasBoll ! hope to merge it

@NicholasBoll
Copy link
Contributor Author

I've removed semicolons from the type definitions files.

Removing semicolons from index.d.ts was enough to not trigger git's threshold on moved files. So now the PR shows removing cli/index.d.ts and adding cli/types/index.d.ts

@bahmutov You'll have to let me know if that's okay. If not, I can remove the semicolon removal as part of a separate PR.

@NicholasBoll
Copy link
Contributor Author

I've added type tests: types/tests/cypress-tests.ts. Running npm run dtslint from inside the cli directory will validate those tests pass.

@NicholasBoll
Copy link
Contributor Author

I have also run npm pack, extracted and linked to that directory. The types seem to work.

@bahmutov
Copy link
Contributor

bahmutov commented Dec 8, 2017

Love it, locally checks out!

@brian-mann
Copy link
Member

Fixes #1046

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.

Refactor TypeScript types and add types for Cypress._, Cypress.$ etc
4 participants