chore: revive type checker #18172

sainthkh · 2021-09-21T01:19:12Z

Closes Fix type checker #18069

User facing changelog

N/A.

It's for internal use.

Additional details

Why was this change necessary? => I don't know exactly when, but it seems that updating listr to listr2 at fix: Migrate from listr to listr2 #16663 broke the type checker.
What is affected by this change? => N/A

Implementation details

I've added some @types packages like @types/webpack-dev-server,
I changed the version of @types/mocha to 8.0.3 everywhere.
As for mocha and jquery types that cause the clash with the bundled types, I deleted the original with patch-package.
- This might cause some build failures. But it can be fixed by building twice.
- I had to reference bundled type files to fix the type problems.
- I had to update patch-package to 6.4.7 to bypass the buffer size bug.
I added css-modules-typescript-loader to add type definitions for the included css files.
I fixed types in the reporter.
I intentionally typed options to any because it's a big change. As we're reusing options argument inside the function code, the names should be changed.
I added types for the functions imported to lodash from underscore.string.
I updated the awesome-typescript-loader test.
I updated type_checker.js to work correctly with listr2.

TODOs for the later PRs

Included type checks for the packages under npm folder and test utils.
Remove options that are given any type. => chore: remove command type todos #20601
Remove create functions like in driver/src/cypress/server.ts, etc. => chore: make create function on server.ts obsolete #18615.

How has the user experience changed?

N/A

PR Tasks

Have tests been added/updated? => Is there a way to check the type_check.js is running correctly to prevent things like this?

cypress-bot · 2021-09-21T01:19:15Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

flotwig

Thank you so much for identifying this issue and fixing it @sainthkh

packages/ui-components/cypress/support/test-entry.scss.d.ts

flotwig · 2021-10-11T17:09:47Z

.../test/support/fixtures/projects/webpack-preprocessor-awesome-typescript-loader/tsconfig.json

@@ -6,6 +6,7 @@
    "sourceMap": false,
    "inlineSourceMap": true,
    "inlineSources": false,
-    "types": ["cypress"]
+    "typeRoots": ["../../../../cli/types"],
+    // "types": ["cypress"]


Suggested change

// "types": ["cypress"]

flotwig · 2021-10-11T17:13:12Z

cli/scripts/post-install.js

+// https://github.com/cypress-io/cypress/issues/18069
+// To avoid type clashes, some files should be commented out entirely by patch-package
+// and uncommented here.
+
+const filesToUncomment = [
+  'mocha/index.d.ts',
+  'jquery/JQuery.d.ts',
+  'jquery/legacy.d.ts',
+  'jquery/misc.d.ts',
+]
+
+filesToUncomment.forEach((file) => {
+  const filePath = join(__dirname, '../types', file)
+  const str = fs.readFileSync(filePath).toString()
+
+  const result = str.split('\n').map((line) => line.substring(3)).join('\n')
+
+  fs.writeFileSync(filePath, result)
+})


I am confused why this is necessary. Can you explain why the commenting/uncommenting is needed?

And can we generate the .patch files instead of inlining them? Would prefer to avoid it if possible.

Unlike other projects, Cypress type definition should copy definition files(d.ts) and bundle them ourselves because of the clash of mocha and jest test functions(i.e. it, describe, etc.). For more info, #7194.

But when we do them, there are 2 copies of mocha and jquery inside our project. Because of that, they cause errors because of the duplicate definitions of global names.

To avoid that, I had to find a way to comment out one of them. I decided to comment the files inside node_modules/@types folder. I did it with patch files added to the project.

But the problem is that those patches are applied earlier than copying them to the cli/types. So, the copied version is the commented version. That's why I had to write code above.

I also don't like this solution and thought about other solutions like below:

Copy the definitions when it is built for production => It seems that there is no prod-build only script. I wasn't confident enough to handle production build script.

Don't use patch and comment them out on post-install.js => It cannot remove the code inside post-install.js. It can comment out already-commented files (i.e. the code will be complicated.)

Creating our own CyMocha, CyJQuery definition. => It was impossible because definitions are tightly coupled. Removing some of them cause unexpected problems.

The conclusion was the code above.

I also hate this. But it seems that this is the simplest solution unless we remove mocha, chai from Cypress or make them optional.

p.s. And this solution is not perfect because it can cause temporary build failures in some environments. For example, I have to build twice on my Ubuntu 20.04 to finish it because the build fails somehow for the type failures.

Blergh. Well, it's certainly an improvement over not having the type-checking at all.

This is the type of thing that will be fixed next time there's an issue anyways, so I'm in favor of merging as-is, even with the kludge.

This is actually causing problems now we are upgrading to TS 4.4, the whole project build completely explodes with invalid types. We should revisit this soonish.

Maybe, the problem could be solved by fixing all the type errors when yarn type-check.

@lmiller1990 I've updated TypeScript to 4.4.4 at #18930. There were some breaking changes at TypeScript. So, it caused many errors.

flotwig · 2021-10-13T23:52:07Z

Whoops, I missed it before merging, but technically this should have been a chore:, not a fix:. Oh well.

sainthkh force-pushed the issue-18069 branch 2 times, most recently from e9d7e4b to b8c64e5 Compare September 29, 2021 02:32

sainthkh added 20 commits October 1, 2021 10:09

Show type errors.

4962c42

Fix web-config type errors.

0a67400

Fix runner-shared.

82ca2f4

fix types.

da76eb2

Fix runner reporter types.

4b60e0e

Fix driver-related runner errors.

6dde25c

fix driver related runner errors.

4e4c259

fix driver-related reporter error

d4fbde5

fix extensions

e33c4da

Fix driver errors.

ea96207

fix server error.

fb9182e

Fix desktop-gui errors.

4a21556

Exit with code 1 on failure.

69f1936

fix npm type errors.

d27bc62

Fix scss typescript errors.

95ba267

Fix type errors.

dcc5f9c

Fix undefined error in type_check.js

081b6fe

Fix failing test.

2b31584

Fix type error.

479fbb2

Type checker renderer test.

b705907

sainthkh force-pushed the issue-18069 branch from b8c64e5 to b705907 Compare October 1, 2021 01:44

Make type-checker work correctly on CI.

d6ca643

sainthkh marked this pull request as ready for review October 1, 2021 02:30

sainthkh requested a review from a team as a code owner October 1, 2021 02:30

sainthkh requested review from flotwig and chrisbreiding and removed request for a team October 1, 2021 02:30

Merge branch 'develop' into issue-18069

b93d092

flotwig suggested changes Oct 11, 2021

View reviewed changes

sainthkh requested a review from flotwig October 12, 2021 02:51

Merge branch 'develop' into issue-18069

49342ed

flotwig approved these changes Oct 13, 2021

View reviewed changes

flotwig merged commit af472b6 into cypress-io:develop Oct 13, 2021

cypress-bot bot mentioned this pull request Oct 13, 2021

Fix type checker #18069

Closed

flotwig changed the title ~~fix: revive type checker~~ chore: revive type checker Oct 13, 2021

flotwig mentioned this pull request Oct 14, 2021

Fix error_ui system test #18497

Closed

lmiller1990 mentioned this pull request Oct 20, 2021

chore: backmerge develop into unified-desktop-gui #18566

Merged

This was referenced Oct 25, 2021

chore: make create function on server.ts obsolete #18615

Merged

chore: refactor create into class $Cy #18715

Merged

sainthkh mentioned this pull request Mar 15, 2022

chore: remove command type todos #20601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: revive type checker #18172

chore: revive type checker #18172

sainthkh commented Sep 21, 2021 •

edited

cypress-bot bot commented Sep 21, 2021

flotwig left a comment

flotwig Oct 11, 2021

flotwig Oct 11, 2021

sainthkh Oct 12, 2021 •

edited

flotwig Oct 13, 2021

lmiller1990 Nov 15, 2021

sainthkh Nov 15, 2021

sainthkh Nov 16, 2021

flotwig commented Oct 13, 2021

chore: revive type checker #18172

chore: revive type checker #18172

Conversation

sainthkh commented Sep 21, 2021 • edited

User facing changelog

Additional details

Implementation details

TODOs for the later PRs

How has the user experience changed?

PR Tasks

cypress-bot bot commented Sep 21, 2021

flotwig left a comment

Choose a reason for hiding this comment

flotwig Oct 11, 2021

Choose a reason for hiding this comment

flotwig Oct 11, 2021

Choose a reason for hiding this comment

sainthkh Oct 12, 2021 • edited

Choose a reason for hiding this comment

flotwig Oct 13, 2021

Choose a reason for hiding this comment

lmiller1990 Nov 15, 2021

Choose a reason for hiding this comment

sainthkh Nov 15, 2021

Choose a reason for hiding this comment

sainthkh Nov 16, 2021

Choose a reason for hiding this comment

flotwig commented Oct 13, 2021

sainthkh commented Sep 21, 2021 •

edited

sainthkh Oct 12, 2021 •

edited