-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Added TypeScript type checker + Fixed type errors. #5780
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
Note: electron-5 fail is a flaky test. |
@sainthkh Some failures in the type checks
|
@jennifer-shehane They're from schema-tools. I wrote PR for that. Please check PR comment above. |
|
copy of chai is unnecessary.
@sainthkh There's some failures from our dtslint
|
|
@sainthkh Sent an invite to give you permission to @cypress/json-schemas |
Opened a PR in cypress-io/json-schemas#526. Things to do:
|
@sainthkh changes should be published under 5.34.1 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍 I love the new type_check
script
@@ -1,4 +1,5 @@ | |||
import sinon from 'sinon' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: stray whitespace maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have added it to separate external dependencies and internal ones. But I think it doesn't matter to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to update dependencies, things are passing for me now! Please do fix any conflicts etc but this looks awesome 👍I will try to investigate getting the new command added to the CI flow for test-runner
# Conflicts: # yarn.lock
- run: | ||
command: yarn type-check --ignore-progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CypressJoseph I added this to CI. Or is there anything more to do?
That’s great! 👍 This should be good to go I think.
…On Mon, Mar 16, 2020 at 5:52 AM Kukhyeon Heo ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In circle.yml
<#5780 (comment)>:
> + - run:
+ command: yarn type-check --ignore-progress
@CypressJoseph <https://github.com/CypressJoseph> I added this to CI. Or
is there anything more to do?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5780 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOVVUCT7CMWMSWSVKIBFZ2TRHXZHRANCNFSM4JQZIRKQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!! 🥂
@sainthkh We're planning to merge this into |
What has been changed?
1. Added type checker done by
tsc
.npm run type-check
command iterates folders inpackages
and check if it hastsconfig.json
file and runstsc
command to check if there is a type error in files. It ignorescli
folder becausedtslint
checks the types.It doesn't completely remove
dtslint
for now because it cannot check the parameter or return type of a variable.This command is added to CI.
Command Options
reporter,runner
).2. Other code changes.
@types
fromcli
. It causes duplicate types and creates a lot of errors. Fixed a new feature introduced in TypeScript 2.9,import('npm-module')
.@types
dependencies ofcli
fromdevDependencies
todependencies
. Because they are used intypes/index.d.ts
.window.NODE
definition.typeRoots
in reporter, runnertsconfig.json
files.Additional details
Currently, CI doesn't check type errors.
When there is a type error, CI will fail.
It uses
tsc --noEmit
to check errors.How has the user experience changed?
N/A
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?