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

TL enforces cypress 4.0.1 #121

Closed
saschahofmann opened this issue Feb 10, 2020 · 31 comments
Closed

TL enforces cypress 4.0.1 #121

saschahofmann opened this issue Feb 10, 2020 · 31 comments

Comments

@saschahofmann
Copy link

  • cypress-testing-library version: 5.3.0
  • node version: 2.14.0
  • npm (or yarn) version: yarn 1.21.1

Relevant code or config
package.json

{
  "name": "cypress-example-docker-gitlab",
  "version": "1.0.0",
  "description": "> Cypress + Docker + GitLabCI = ❤️",
  "main": "index.js",
  "private": true,
  "scripts": {
    "test": "cypress run"
  },
  "repository": {
    "type": "git",
    "url": "git+ssh://git@gitlab.com/cypress-io/cypress-example-docker-gitlab.git"
  },
  "keywords": [
    "cypress",
    "cypress-io",
    "example",
    "gitlab"
  ],
  "author": "gleb@cypress.io",
  "license": "ISC",
  "bugs": {
    "url": "https://gitlab.com/cypress-io/cypress-example-docker-gitlab/issues"
  },
  "homepage": "https://gitlab.com/cypress-io/cypress-example-docker-gitlab#README",
  "devDependencies": {
    "cypress": "3.8.3",
    "@testing-library/cypress": "^5.3.0"
  }
}

What you did:
I cloned the cypress gitlab example and added the testing library cypress dependency (as seen in the package.json above) and bumped the cypress version to 3.8.3. I then run cypress in a docker container:
docker run --rm -it -v /<path to repo>/:/<new dir>/ cypress/browsers:node12.14.0-chrome79-ff71 bash
What happened:
While doing yarn inside the container I receive an error! Apparently, yarn tries to install two versions of Cypress (3.8.3 and 4.0.1). I remove the TL dependency the error disappears. Not sure whether this expected behaviour or whether the provided peerDependency in TL should handle this?

 The Cypress App could not be downloaded.

Does your workplace require a proxy to be used to access the Internet? If so, you must configure the HTTP_PROXY environment variable before downloading Cypress. Read more: https://on.cypress.io/proxy-configuration

Otherwise, please check network connectivity and try again:

----------

URL: https://download.cypress.io/desktop/4.0.1?platform=linux&arch=x64
Error: Corrupted download

Expected downloaded file to have checksum: 7e26904b56c560f9ba893085903b8435c651a86058b4ba81075f51158e4846e884a039c473d07ce74ef93ab8134164e8fe6eac192a8fababe99fc13ce757310e
Computed checksum: 12c7e488d3ff1f7452979b64972d7ed3b12ca127f2f388abdf816660509fa37034aae50256d91da0affb9eeefaad4fd01eff07e60839e15513516c4af02f869a

Expected downloaded file to have size: 167819701
Computed size: 167819701

----------

Platform: linux (Debian - 9.11)
Cypress Version: 4.0.1
The Cypress App could not be downloaded.

Does your workplace require a proxy to be used to access the Internet? If so, you must configure the HTTP_PROXY environment variable before downloading Cypress. Read more: https://on.cypress.io/proxy-configuration

Reproduction repository:

https://gitlab.com/cypress-io/cypress-example-docker-gitlab

@saschahofmann
Copy link
Author

saschahofmann commented Feb 10, 2020

Is this related to the fact that I am trying to run this inside the cypress docker container?
Edit: It behaves the same locally on MacOs!

@saschahofmann
Copy link
Author

Another obervation: if I don't add the package but run yarn first and then add the TL the install command doesn't throw an error but I still end up with two Cypress versions in /root/.cache/Cypress

@saschahofmann
Copy link
Author

I tried to rollback my TL version but even on 5.0.2 it forces a second install of cypress 4.0.1

@NicholasBoll
Copy link
Contributor

Thanks for filing this issue!

The repository you linked doesn't include this library. Do you have a link to a repo that does?

I did run into your exact issue myself, but I don't think it was because of this library. I think it was because of @types/testing-library__cypress. This library includes cypress as a peer dependency stating that multiple versions of Cypress are valid. DefinitelyTyped does not allow peer dependencies, so the dependency of Cypress is * there: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/testing-library__cypress/package.json#L4

It seems like yarn is resolving the * to be the latest for some reason if your local version isn't compatible with the latest.

My workaround was to set a resolutions in the package.json file. Later I just upgraded my local Cypress to v4 and it no longer duplicated.

I don't think the issue is with this library, but with how yarn resolves and the limitations DefinitelyTyped imposes on type definitions (can't have peerDependency or devDependency)

Can you confirm if you're project is using @types/testing-library__cypress? Or if the above workarounds work for you?

@valscion
Copy link

valscion commented Feb 11, 2020

I am facing the same issue. The problem does indeed stem from how @types/testing-library__cypress sets up a dependency on cypress in its package.json: https://unpkg.com/browse/@types/testing-library__cypress@5.0.3/package.json

I'm fairly certain we could fix it with using resolutions to force @types/testing-library__cypress to use the same version of Cypress as we have declared in our own package.json.

It's a shame that the types declared in @testing-library/cypress package.json causes cypress to be installed again with a different version, when we're upgrading the @testing-library/cypress package:

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

So kinda similar to #98 and unfortunately DefinitelyTyped/DefinitelyTyped#41917 wasn't enough to make yarn not install duplicate versions of cypress. So our CI is failing when upgrading.

It's even more unfortunate given we do not use TypeScript but we get to this dependency maintentance problem because of this library having a dependency on the @types/testing-library__cypress package 😩.

I however do understand why the package is listed in package.json and do not want it to be removed.

An unfortunate dependency management issue indeed 😕

@valscion
Copy link

valscion commented Feb 11, 2020

Yup, adding:

  "resolutions": {
    "cypress": "^3.8.3"
  },

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

to our package.json made Yarn install the same version of cypress we have specified in devDependencies:

js__Update__testing-library_cypress__5_0_2_→_5_3_0__minor__by_depfu·Pull_Request__6542·_venuu_venuu

And cypress v4.0.1 is no longer installed.

@saschahofmann
Copy link
Author

@NicholasBoll Sorry. I used the linked repo as a base and only modified the package.json as shown above. Will make that clearer next time. I don't have a direct dependency on @types/testing-library__cypress but it is installed. I assume as dependency of TL.

I will try your proposed 'resolution solution'. TY

@NicholasBoll
Copy link
Contributor

Ah. I see: https://github.com/testing-library/cypress-testing-library/blob/master/package.json#L46

This library has a dependency on @types/testing-library__cypress... That's probably to make it easier for people... You don't have to install the types yourself...

Unfortunately DefinitelyTyped does not allow you to require devDependencies or peerDependencies which is what we really want to do. DefinitelyTyped requires any dependencies to be direct dependencies. For whatever reason, yarn doesn't pick a common version that satisfies all requirements. It instead recognizes that @types/testing-library__cypress requires "cypress": "*" and your project requires 3.8.x and resolves to 4.0.1 and 3.8.3 respectively...

I'm not sure how to get around that other than this package not requiring the type definitions. Technically the types aren't "required". They would be required only for Typescript projects. Typescript projects would have the same issue as listed here if they are also using Cypress@3.x. I upgraded a project to Cypress@4 without issue. That's another option.

If anyone feels strongly that this package should not rely on @types/testing-library__cypress, please open an issue and maintainers can discuss what that means. Chances are that's a breaking change for Typescript users (they'd have to update their package.json to include the types package since it would no longer be included by this project).

Otherwise, resolutions or upgrading Cypress is a workaround...

It does seem funny that technically a major bump to Cypress broke things even without upgrading anything else...

@saschahofmann
Copy link
Author

Yes. Really peculiar that one can install the same package.json two days apart and suddenly it starts breaking because it installs two different versions. Unfortunately, we rely on another library which hasn't been upgraded but until then the resolutions works so happy to close.

@saschahofmann
Copy link
Author

You think it's worth mentioning this to the guys from DefinitelyTyped?

@NMinhNguyen
Copy link

Is the issue here that @types/testing-library__cypress has a dependency on cypress so that it can access its types? A tricky one indeed. Was about to suggest not adding cypress to dependencies at all (kinda how react-dom is meant to be used with react but doesn't list it as a dependency), but that might (might because I don't know for sure 🙂) have implications on tools like Yarn PnP where all dependencies should be listed and implicit ones are disallowed.. 😞

@NicholasBoll
Copy link
Contributor

Is the issue here that @types/testing-library__cypress has a dependency on cypress so that it can access its types? A tricky one indeed. Was about to suggest not adding cypress to dependencies at all (kinda how react-dom is meant to be used with react but doesn't list it as a dependency), but that might (might because I don't know for sure 🙂) have implications on tools like Yarn PnP where all dependencies should be listed and implicit ones are disallowed.. 😞

Unfortunately the type definitions for this library are on DefinitelyTyped and DefinitelyTyped enforces a restriction on dependencies there. A type definition cannot use devDependencies or peerDependencies: DefinitelyTyped/DefinitelyTyped#20290

The only actionable item from this library would be removing the dependency on the type definitions.

There is nothing actionable for the maintainers of @types/testing-library__cypress. It is an imposed limitation on the maintainers of DefinitelyTyped.

The only thing users can do if they hit this issue is to use resolutions if they use yarn, or go through the upgrade process for Cypress

@NicholasBoll
Copy link
Contributor

You think it's worth mentioning this to the guys from DefinitelyTyped?

@saschahofmann If you feel strongly that this library should not have a dependency on the type definitions (breaking change), open a PR to suggest removing that dependency.

You can open an issue on DefinitelyTyped referencing the @types/testing-library__cypress, but I'm not sure that will go anywhere since this issue stems from DefinitelyTyped's CI restrictions.

Type definitions used to be in this library, but were separated to allow Types and runtime to move independently since the source code here is not in Typescript.

@saschahofmann
Copy link
Author

I actually think the resolution solution is fine especially since I very much like to have the type definitions! I will just wait until the last dependency updates to 4 and this should not even be necessary anymore. Happy to close.

@NMinhNguyen
Copy link

So to recap, DT won’t allow simply removing the dependency on cypress altogether?

@NicholasBoll
Copy link
Contributor

So to recap, DT won’t allow simply removing the dependency on cypress altogether?

Correct. The @types/testing-library__cypress types do depend on Cypress.Chainable and will fail if they are not installed. I don't know of a mechanism in the DefinitelyTyped ecosystem to bring in a dependency without it also being in the package.json file

@kentcdodds
Copy link
Member

Yeah, I tried every variant of dependencies in the package.json in DefinitelyTyped and removing it entirely failed their build.

Another thing to consider is bringing the types back into this repo. I know that I don't have the bandwidth to keep up with things, but @NicholasBoll (or anyone else) if you can commit to maintain them then I'd be ok with that.

@NicholasBoll
Copy link
Contributor

I'd be okay with bringing types back to this repository. I have a branch that was going to update the types on DefinitelyTyped to add a bit more DX to the JSDocs. Perhaps we'll consider bundling that with the next major bump.

@NMinhNguyen
Copy link

Perhaps we'll consider bundling that with the next major bump.

@NicholasBoll do you mean bundling the types? Curious why bundling would be deemed a breaking change to necessitate waiting until the next major. Maybe I'm misunderstanding something 😅

@NicholasBoll
Copy link
Contributor

I don't know if it will actually break for people, but it could potential break something. There are some other breaking changes we have planned that this could get rolled up into

@NMinhNguyen
Copy link

I don't know if it will actually break for people, but it could potential break something.

We could in theory roll back the changes in a patch release if it ends up being breaking. But I guess the current resolutions workaround suffices.

@stevenmusumeche
Copy link

Just as an FYI - we use a Docker image with Cypress already installed in CI, and thus cannot use this library at all because it installs Cypress (due to listing the @types package as a dependency).

@kentcdodds
Copy link
Member

I'm fine if someone wants to bring the typings back and even write the whole thing in typescript. I just don't have the bandwidth to do it myself.

@stevenmusumeche
Copy link

Have you considering dropping the @types dependency entirely? That would solve this problem. That's how other packages handle this AFAIK. TypeScript users understand that they need to install the @types package for Javascript libraries.

@kentcdodds
Copy link
Member

The point is to make it so nobody has to install anything extra to get typings (even regular JavaScript users).

@stevenmusumeche
Copy link

I understand the intent but it’s causing more problems than it solves. TS users know how to get types.

@kentcdodds
Copy link
Member

I understand your position and I've suggested a solution. I don't have the bandwidth to work on it right now, but I'd welcome anyone to make a PR to internalize the types or even rewrite to TypeScript if they want to. I think that removing the dependency would be a breaking change and I'm not prepared to do that at the moment.

@eliw00d
Copy link

eliw00d commented Jun 18, 2020

We have Cypress as an optional dependency to avoid failed downloads crashing our CI so we are not able to use this library due to being forced to download whatever version the @types dependency decides. 😞

@kentcdodds
Copy link
Member

Hi @eliw00d,

I'm sorry to hear that. I'm sure that's frustrating.

I'd welcome anyone to make a PR to internalize the types or even rewrite to TypeScript if they want to.

Feel free to make a PR :)

@Carl-Foster
Copy link

I've been able to manage this using yarn berry and adding in the packageExtensions config to .yarnrc.yml.

packageExtensions:
  "@types/testing-library__cypress@*":
    dependencies:
      "cypress": "4.10.0" # current version installed

@kentcdodds
Copy link
Member

Type Defs are now built-in, so this shouldn't be a problem anymore.

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

No branches or pull requests

8 participants