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

chore: revive type checker #18172

Merged
merged 23 commits into from Oct 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/package.json
Expand Up @@ -74,7 +74,7 @@
"@types/jquery": "3.3.31",
"@types/lodash": "4.14.168",
"@types/minimatch": "3.0.3",
"@types/mocha": "5.2.7",
"@types/mocha": "8.0.3",
"@types/sinon": "7.5.1",
"@types/sinon-chai": "3.2.5",
"chai": "3.5.0",
Expand Down
21 changes: 21 additions & 0 deletions cli/scripts/post-install.js
Expand Up @@ -5,6 +5,7 @@

const { includeTypes } = require('./utils')
const shell = require('shelljs')
const fs = require('fs')
const { join } = require('path')
const resolvePkg = require('resolve-pkg')

Expand Down Expand Up @@ -72,3 +73,23 @@ shell.sed('-i', 'from \'sinon\';', 'from \'../sinon\';', sinonChaiFilename)
// copy experimental network stubbing type definitions
// so users can import: `import 'cypress/types/net-stubbing'`
shell.cp(resolvePkg('@packages/net-stubbing/lib/external-types.ts'), 'types/net-stubbing.ts')

// 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)
})
Comment on lines +77 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sainthkh sainthkh Oct 12, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

5 changes: 3 additions & 2 deletions cli/types/cy-bluebird.d.ts
@@ -1,11 +1,12 @@
// Shim definition to export a namespace. Cypress is actually a global module
// so import/export isn't allowed there. We import here and define a global module
// so that Cypress can get and use the Blob type
import BluebirdStatic = require('./bluebird')
import ImportedBluebird = require('./bluebird')

export = Bluebird
export as namespace Bluebird

declare namespace Bluebird {
type BluebirdStatic = typeof BluebirdStatic
type BluebirdStatic = typeof ImportedBluebird
interface Promise<T> extends ImportedBluebird<T> {}
}
9 changes: 7 additions & 2 deletions cli/types/cypress.d.ts
Expand Up @@ -170,6 +170,11 @@ declare namespace Cypress {
*/
interface ApplicationWindow { } // tslint:disable-line

/**
* The configuration for Cypress.
*/
type Config = ResolvedConfigOptions & RuntimeConfigOptions

/**
* Several libraries are bundled with Cypress by default.
*
Expand Down Expand Up @@ -297,7 +302,7 @@ declare namespace Cypress {
/**
* Fire automation:request event for internal use.
*/
automation(eventName: string, ...args: any[]): Promise<any>
automation(eventName: string, ...args: any[]): Bluebird.Promise<any>

/**
* Promise wrapper for certain internal tasks.
Expand All @@ -313,7 +318,7 @@ declare namespace Cypress {
// {defaultCommandTimeout: 10000, pageLoadTimeout: 30000, ...}
```
*/
config(): ResolvedConfigOptions & RuntimeConfigOptions
config(): Config
/**
* Returns one configuration value.
* @see https://on.cypress.io/config
Expand Down
@@ -1,3 +1,5 @@
/// <reference path="../../../../../cli/types/mocha/index.d.ts" />

import * as babel from '@babel/core'
import { expect } from 'chai'
import { createTransformPluginsFileBabelPlugin } from './babelTransform'
Expand Down
2 changes: 2 additions & 0 deletions npm/cypress-schematic/src/schematics/ng-add/index.spec.ts
@@ -1,3 +1,5 @@
/// <reference path="../../../../../cli/types/mocha/index.d.ts" />

import { SchematicTestRunner, UnitTestTree } from '@angular-devkit/schematics/testing'
import { join, resolve } from 'path'
import { expect } from 'chai'
Expand Down
4 changes: 2 additions & 2 deletions package.json
Expand Up @@ -90,7 +90,7 @@
"@types/glob": "7.1.1",
"@types/lodash": "4.14.168",
"@types/markdown-it": "0.0.9",
"@types/mini-css-extract-plugin": "1.4.2",
"@types/mini-css-extract-plugin": "1.2.3",
"@types/mocha": "8.0.3",
"@types/node": "14.14.31",
"@types/prismjs": "1.16.0",
Expand Down Expand Up @@ -165,7 +165,7 @@
"mock-fs": "4.9.0",
"odiff-bin": "2.1.0",
"parse-github-repo-url": "1.4.1",
"patch-package": "6.2.2",
"patch-package": "6.4.7",
"plist": "3.0.1",
"pluralize": "8.0.0",
"postinstall-postinstall": "2.0.0",
Expand Down
7 changes: 7 additions & 0 deletions packages/desktop-gui/src/main.scss.d.ts
@@ -0,0 +1,7 @@
// This file is automatically generated.
// Please do not change this file!
interface CssExports {

}
export const cssExports: CssExports;
export default cssExports;
1 change: 1 addition & 0 deletions packages/driver/cypress/integration/dom/visibility_spec.ts
@@ -1,3 +1,4 @@
// @ts-ignore
const { $, dom } = Cypress

describe('src/cypress/dom/visibility', () => {
Expand Down
5 changes: 3 additions & 2 deletions packages/driver/index.d.ts
@@ -1,6 +1,7 @@
/// <reference path="../../cli/types/index.d.ts" />
/// <reference path="../ts/index.d.ts" />
/// <reference path="../../cli/types/jquery/index.d.ts" />

export const $Cypress: Cypress.Cypress

export const $: typeof JQuery
export const $: JQuery
export default $Cypress
12 changes: 12 additions & 0 deletions packages/driver/index.ts
@@ -1,3 +1,15 @@
/// <reference path="../../cli/types/chai-jquery/index.d.ts" />
/// <reference path="../../cli/types/sinon-chai/index.d.ts" />
/// <reference path="../../cli/types/cypress-expect.d.ts" />

/// <reference path="../ts/index.d.ts" />

declare global {
interface Window {
Cypress: Cypress.Cypress
}
}

import $Cypress from './src/main'

export default $Cypress
Expand Down
2 changes: 2 additions & 0 deletions packages/driver/package.json
Expand Up @@ -26,8 +26,10 @@
"@sinonjs/fake-timers": "7.0.2",
"@types/chalk": "^2.2.0",
"@types/common-tags": "^1.8.0",
"@types/jquery.scrollto": "1.4.29",
"@types/lodash": "^4.14.168",
"@types/mocha": "^8.0.3",
"@types/underscore.string": "0.0.38",
"angular": "1.8.0",
"basic-auth": "2.0.1",
"blob-util": "2.0.2",
Expand Down
15 changes: 12 additions & 3 deletions packages/driver/src/config/jquery.scrollto.ts
Expand Up @@ -8,6 +8,8 @@
* @version 2.1.3
*/

/// <reference types="jquery.scrollto" />

/** eslint-disable */
import $ from 'jquery'

Expand Down Expand Up @@ -109,7 +111,7 @@ export function scrollTo (target, duration, settings) {
let max = $scrollTo.max(elem, axis)

if (toff) { // jQuery / DOMElement
attr[key] = toff[pos] + (win ? 0 : prev - $elem.offset()[pos])
attr[key] = toff[pos] + (win ? 0 : prev - $elem.offset()![pos])

// If it's a dom element, reduce the margin
if (settings.margin) {
Expand Down Expand Up @@ -192,18 +194,25 @@ function both (val) {
return isFunction(val) || $.isPlainObject(val) ? val : { top: val, left: val }
}

// TODO: find the type of _last in the JQuery code.
interface Tween extends JQuery.Tween<JQuery.Node> {
_last: any
}

// Add special hooks so that window scroll properties can be animated
$.Tween.propHooks.scrollLeft =
$.Tween.propHooks.scrollTop = {
get (t) {
return $(t.elem)[t.prop]()
},
set (t) {
set (t: Tween) {
let curr = this.get(t)

// If interrupt is true and user scrolled, stop animating
if (t.options.interrupt && t._last && t._last !== curr) {
return $(t.elem).stop()
$(t.elem).stop()

return
}

let next = Math.round(t.now)
Expand Down
9 changes: 9 additions & 0 deletions packages/driver/src/config/lodash.ts
Expand Up @@ -10,3 +10,12 @@ _.mixin({
})

export default _

declare module 'lodash' {
interface LoDashStatic {
clean: typeof clean
count: typeof count
isBlank: typeof isBlank
toBoolean: typeof toBoolean
}
}
13 changes: 12 additions & 1 deletion packages/driver/src/cy/commands/actions/click.ts
Expand Up @@ -33,10 +33,21 @@ const formatMouseEvents = (events) => {
})
}

// TODO: remove any, Function, Record
type MouseActionOptions = {
subject: any
positionOrX: string | number
y: number
userOptions: Record<string, any>
onReady: Function
onTable: Function
defaultOptions?: Record<string, any>
}

export default (Commands, Cypress, cy, state, config) => {
const { mouse, keyboard } = cy.devices

const mouseAction = (eventName, { subject, positionOrX, y, userOptions, onReady, onTable, defaultOptions }) => {
const mouseAction = (eventName, { subject, positionOrX, y, userOptions, onReady, onTable, defaultOptions }: MouseActionOptions) => {
let position
let x

Expand Down
6 changes: 4 additions & 2 deletions packages/driver/src/cy/commands/actions/focus.ts
Expand Up @@ -7,7 +7,8 @@ import $elements from '../../../dom/elements'

export default (Commands, Cypress, cy) => {
return Commands.addAll({ prevSubject: ['element', 'window'] }, {
focus (subject, options = {}) {
// TODO: any -> Partial<Cypress.Loggable & Cypress.Timeoutable>
focus (subject, options: any = {}) {
const userOptions = options

// we should throw errors by default!
Expand Down Expand Up @@ -84,7 +85,8 @@ export default (Commands, Cypress, cy) => {
return verifyAssertions()
},

blur (subject, options = {}) {
// TODO: any -> Partial<Cypress.BlurOptions>
blur (subject, options: any = {}) {
const userOptions = options

// we should throw errors by default!
Expand Down
27 changes: 19 additions & 8 deletions packages/driver/src/cy/commands/actions/scroll.ts
Expand Up @@ -30,7 +30,8 @@ const isNaNOrInfinity = (item) => {

export default (Commands, Cypress, cy, state) => {
Commands.addAll({ prevSubject: 'element' }, {
scrollIntoView (subject, options = {}) {
// TODO: any -> Partial<Cypress.ScrollToOptions>
scrollIntoView (subject, options: any = {}) {
const userOptions = options

if (!_.isObject(userOptions)) {
Expand Down Expand Up @@ -114,6 +115,9 @@ export default (Commands, Cypress, cy, state) => {
const scrollIntoView = () => {
return new Promise((resolve, reject) => {
// scroll our axes
// TODO: done() came from jQuery animate(), specifically, EffectsOptions at misc.d.ts
// The type definition should be fixed at @types/jquery.scrollto.
// @ts-ignore
return $(options.$parent).scrollTo(options.$el, {
axis: options.axis,
easing: options.easing,
Expand All @@ -132,7 +136,7 @@ export default (Commands, Cypress, cy, state) => {
},
always () {
if (parentIsWin) {
return delete options.$parent.contentWindow
delete options.$parent.contentWindow
}
},
})
Expand All @@ -153,7 +157,8 @@ export default (Commands, Cypress, cy, state) => {
})

Commands.addAll({ prevSubject: ['optional', 'element', 'window'] }, {
scrollTo (subject, xOrPosition, yOrOptions, options = {}) {
// TODO: any -> Partial<Cypress.ScrollToOptions>
scrollTo (subject, xOrPosition, yOrOptions, options: any = {}) {
let x; let y
let userOptions = options

Expand All @@ -168,7 +173,7 @@ export default (Commands, Cypress, cy, state) => {
y = yOrOptions
}

let position = null
let position: string | null = null

// we may be '50%' or 'bottomCenter'
if (_.isString(xOrPosition)) {
Expand Down Expand Up @@ -293,7 +298,7 @@ export default (Commands, Cypress, cy, state) => {
$utils.filterOutOptions(options, { duration: 0, easing: 'swing' }),
)

const messageArgs = []
const messageArgs: string[] = []

if (position) {
messageArgs.push(position)
Expand All @@ -306,12 +311,12 @@ export default (Commands, Cypress, cy, state) => {
messageArgs.push(deltaOptions)
}

const log = {
const log: Record<string, any> = {
message: messageArgs.join(', '),
timeout: options.timeout,
consoleProps () {
// merge into consoleProps without mutating it
const obj = {}
const obj: Record<string, any> = {}

if (position) {
obj.Position = position
Expand Down Expand Up @@ -357,10 +362,16 @@ export default (Commands, Cypress, cy, state) => {
const scrollTo = () => {
return new Promise((resolve, reject) => {
// scroll our axis'
// TODO: done() came from jQuery animate(), specifically, EffectsOptions at misc.d.ts
// The type definition should be fixed at @types/jquery.scrollto.
// @ts-ignore
$(options.$el).scrollTo({ left: x, top: y }, {
axis: options.axis,
easing: options.easing,
duration: options.duration,
// TODO: ensureScrollable option does not exist on jQuery or config/jquery.scrollto.ts.
// It can be removed.
// @ts-ignore
ensureScrollable: options.ensureScrollable,
done () {
return resolve(options.$el)
Expand All @@ -376,7 +387,7 @@ export default (Commands, Cypress, cy, state) => {
})

if (isWin) {
return delete options.$el.contentWindow
delete options.$el.contentWindow
}
})
}
Expand Down