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

[@testing-library/cypress] Add JQuery types from cypress #40210

Open
wants to merge 2 commits into
base: master
from

Conversation

@AlgusDark
Copy link

AlgusDark commented Nov 7, 2019

This PR fix #40099 by using the types from cypress/types,

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
AlgusDark added 2 commits Nov 7, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 7, 2019

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

master #40210 diff
Batch compilation
Memory usage (MiB) 144.4 141.6 -2.0%
Type count 34128 34128 0%
Assignability cache size 37789 37789 0%
Language service
Samples taken 81 81 0%
Identifiers in tests 81 81 0%
getCompletionsAtPosition
    Mean duration (ms) 390.0 594.6 +52.5% 🔸
    Mean CV 13.8% 11.2%
    Worst duration (ms) 462.6 716.6 +54.9% 🔸
    Worst identifier cy container
getQuickInfoAtPosition
    Mean duration (ms) 380.2 580.7 +52.7% 🔸
    Mean CV 13.5% 13.2% -2.2%
    Worst duration (ms) 453.9 710.4 +56.5% 🔸
    Worst identifier findByText $container

It looks like there are several metrics that changed quite a bit. You might want to take a look and make sure your changes won’t cause slow-downs for users consuming these types.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 7, 2019

@AlgusDark Thank you for submitting this PR!

🔔 @aaronmcadam @bastibuck @NoriSte @wKovacs64 @existentialism @airato @simjes - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Nov 12, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 12, 2019

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@wKovacs64

This comment has been minimized.

Copy link
Contributor

wKovacs64 commented Nov 15, 2019

My concern is that the Cypress docs mention using the types option to avoid conflicts:

The types will tell the TypeScript compiler to only include type definitions from Cypress. This will address instances where the project also uses @types/chai or @types/jquery. Since Chai and jQuery are namespaces (globals), incompatible versions will cause the package manager (yarn or npm) to nest and include multiple definitions and cause conflicts.

I suspect if your project used @types/jquery and the version didn't match the jQuery types bundled with Cypress, you'd run into conflicts by importing like that. I think that should be confirmed/denied before merging.

@NicholasBoll or @jennifer-shehane might have a more informed opinion here as they authored the aforementioned docs on using TypeScript with Cypress.

Copy link
Contributor

sheetalkamat left a comment

Waiting to hear from the owners of cypress

@typescript-bot typescript-bot moved this from Review to Needs Author Attention in Pull Request Status Board Nov 18, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 18, 2019

@AlgusDark One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Pull Request Status Board
Needs Author Attention
4 participants
You can’t perform that action at this time.