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

Include typescript definitions 856 #979

Merged
merged 8 commits into from
Nov 30, 2017

Conversation

bahmutov
Copy link
Contributor

  • brings in @types/cypress definition file into NPM repo
  • links definition file index.d.ts to cypress package by specifying it in the CLI package.json file as "types": "index.d.ts"
  • updates definition bringing it up to date (well, work in progress)
  • adds a lot more documentation from API docs to the JSdoc comments to make available to IntelliSense

cli/package.json Outdated
@@ -18,6 +18,7 @@
"lint": "bin-up eslint --fix *.js bin/* lib/*.js lib/**/*.js test/*.js test/**/*.js",
"prebuild": "npm run test-dependencies && node ./scripts/start-build.js",
"build": "node ./scripts/build.js",
"postbuild": "cp index.d.ts build",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this just be in start-build.js ? no reason to add it here

@brian-mann brian-mann merged commit 1b89e31 into develop Nov 30, 2017
bahmutov referenced this pull request in DefinitelyTyped/DefinitelyTyped Nov 30, 2017
Add the missing RegExp definition for the url params of the route function. Also allow to call the route with the options params.
@NicholasBoll
Copy link
Contributor

I noticed the @types/cypress types were out of date (pre-1.0 release) so I started working on a local version with plans to update DefinitelyTyped later.

The typings included here aren't perfect (ex: Command.add is the new API vs Command.addParentCommand). Also Chainable is more nuanced. There are really a parent command and 2 chainable routes: generic and jQuery. I could definitely make a PR that fixes these issues.

I'm happy to see type definitions move to the source repo and be official. I'm not sure how I feel about this move though. In my project I don't locally install Cypress (against warnings) because it increases the size of node_modules by over 350MB for developers and adds a lot of extra time to CI (if the workspace is cleared, Cypress has to download, unzip and install every time). If I don't locally install, I don't get these type definitions. Maybe a mitigation is to periodically just copy/paste these type definitions from here to DefinitelyTyped?

In any case, I'm interested in contributing to type definitions.

@brian-mann
Copy link
Member

@NicholasBoll we just had a new PR land that fixes some of the issues you documented. #1010

Not sure what you mean by "if the workspace is cleared Cypress has to download, unzip, install"

We cache everything in our own CI to specifically avoid this. Not sure which provider you are using. Also this sounds like a huge PITA if you're not using node_modules to control the version of Cypress in CI. We're releasing quite frequently these days. How are you doing that? And how can you also synchronize that version with other developers?

It's likely possible that you could check cypress in as a devDependency, but tell it not to download the binary -- and we could likely add a new command line flag to the CLI that points it at a binary at a different location.

Unfortunately, you'd have to add the no download flag every time during npm install to avoid the binary installation.

As a sidenote, we have discussed allowing users to download a non electron version of Cypress that runs strictly in node with no GUI - but that would mostly just affect CI. Likely you'd still want to use the desktop app locally.

@NicholasBoll
Copy link
Contributor

I'm using Jenkins with Docker (from https://github.com/blacklabelops/jenkins). It isn't hard to keep up - just build a new image, upload and upgrade the swarm. Also the official download prompts upgrades so it isn't hard for developers to keep up-to-date.

The best way to ensure consistent builds a clean directory with a fresh yarn install (assuming a container instance is reused for multiple jobs). If Cypress is a devDependency in package.json, it goes through the download, install, unzip steps each time:

> cypress@1.1.2 postinstall {PATH_TO_PROJECT}/node_modules/cypress
> node index.js --exec install

Installing Cypress (version: 1.1.2)

 ✔  Downloaded Cypress
 ✔  Unzipped Cypress
 ✔  Finished Installation {PATH_TO_PROJECT}/node_modules/cypress/dist/Cypress.app

You can now open Cypress by running: node_modules/.bin/cypress open

I'm curious to know how that could be cached (maybe symlink the node_modules/cypress/dist folder before running npm/yarn install?). Our docker containers have Cypress installed globally npm i cypress -g so a CI run just needs to git checkout the branch and run a yarn install and then start a Cypress run. I see caching being done if a npm/yarn install is done without clearing the contents of node_modules. If Cypress is installed globally, it is also possible to just copy the cypress directory and run cypress run from that directory (assuming the baseUrl is valid) from CI further decreasing setup time (we run multiple nodes simultaneously).

I think it would be fine for a env variable to point to a binary for the postinstall step in Cypress. If provided and valid it would skip downloading the binary. Nearly all time installing Cypress is in that postinstall step (downloading and extracting the binary) and about 333MB on disk is from the dist directory where the binary is placed.

I think a local install with an optional global binary would be ideal.

@jennifer-shehane jennifer-shehane deleted the include-typescript-definitions-856 branch July 24, 2018 16:54
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