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

Build: add type check using typescript --checkJs #2267

Merged
merged 2 commits into from May 14, 2019

Conversation

Projects
None yet
2 participants
@golopot
Copy link
Contributor

commented May 10, 2019

No description provided.

@ljharb
Copy link
Collaborator

left a comment

These fixes are fine, but is there some way we could use CI to ensure they stay correct? otherwise i'm not sure they're adding value.

@golopot

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

This errors are found by typescript's --checkJs feature. It pull types from jsdoc and typecheck. It is great. CI can be setup by running tsc.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Let's add an npm run-script for that, then, and a separate travis.yml job that does it, and the appropriate dev deps.

golopot added some commits May 11, 2019

@golopot golopot force-pushed the golopot:fix-some-jsdoc-errors branch from b189e12 to 084c678 May 11, 2019

@golopot golopot changed the title [Chore]: fix some jsdoc errors Build: add type check using typescript --checkJs May 11, 2019

@golopot golopot force-pushed the golopot:fix-some-jsdoc-errors branch from 084c678 to d344dca May 11, 2019

/* Strict Type-Checking Options */
// "strict": true, /* Enable all strict type-checking options. */
// "noImplicitAny": false, /* Raise error on expressions and declarations with an implied 'any' type. */
// "strictNullChecks": true, /* Enable strict null checks. */

This comment has been minimized.

Copy link
@golopot

golopot May 11, 2019

Author Contributor

The flag strictNullChecks is left unenabled because there is about 20 places where I need to either use @ts-ignore or write very awkward codes.

@golopot

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

It might make sense to add eslint-plugin-jsdoc. The errors it catches includes: 1. Missing or superfluous jsdoc param. 2. Unmatched Jsdoc param name and actual name. 3. Enforce presence of types annotation.

Show resolved Hide resolved lib/rules/jsx-closing-bracket-location.js Outdated
Show resolved Hide resolved lib/rules/jsx-closing-bracket-location.js Outdated
Show resolved Hide resolved lib/rules/no-unknown-property.js Outdated
Show resolved Hide resolved lib/rules/no-unused-state.js Outdated
Show resolved Hide resolved lib/types.d.ts Outdated
@@ -3,6 +3,7 @@
*/
'use strict';

/** @type {any[]}} */

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

string[]?

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

Tsc does not accept string[]. The official type declaration requires that in a.concat(b), a and b must have the same type.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

when is this array concatted with something that's not also a string[]?

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

In lines bellow it is concatted with setting.something.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

which should be a string[] per the schema, no?

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

By schema it could be {name: something, linkAttribute: something}[]

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

ah - then should the type be (string | { name: string, linkAttribute: string })[]?

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

But settings.linkComponents is not validated, it could be anything.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

if it's not that schema tho, the plugin won't work properly, so i think we can assume that's its type.

@@ -172,6 +174,7 @@ module.exports = function propTypesInstructions(context, components, utils) {
UnionTypeAnnotation: function(annotation, parentName, seen) {
const unionTypeDefinition = {
type: 'union',
/** @type {any} */

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

lets avoid any use of any :-)

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

Tsc does not accept any[] and any[]|true.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

unknown[]? unknown[] | true? you can certainly do any of those in TS, altho maybe not in jsdoc.

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

Tsc does not accept unknown[] unknown[]|true also.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

just in jsdoc? it certainly works in TS.

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

unionTypeDefinition.children is at a time assigned true, so it cannot be unknown[]. unionTypeDefinition.children.length is used, so it cannot be unknown[]|true.

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

then that implies there's a bug, or else type narrowing would have removed true prior to the .length access?

@golopot golopot closed this May 12, 2019

@golopot golopot force-pushed the golopot:fix-some-jsdoc-errors branch from 084c678 to fce69eb May 12, 2019

@golopot

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

I somehow closed this by accident. Apologies.

@golopot golopot reopened this May 12, 2019

Show resolved Hide resolved lib/rules/jsx-closing-bracket-location.js Outdated
Show resolved Hide resolved package.json Outdated
@ljharb

ljharb approved these changes May 12, 2019

Copy link
Collaborator

left a comment

I've explicitly commented on all non-comment changes, just to be thorough.

I'd still like to avoid any use of any.

LGTM pending https://github.com/yannickcr/eslint-plugin-react/pull/2267/files#r283097857

@@ -127,7 +127,7 @@ module.exports = {
handleSFCUsage(node);
}
if (classComponent) {
handleClassUsage(node, classComponent);
handleClassUsage(node);

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

This is a harmless extra arg being passed - good to clean up. semver-patch.

@@ -193,7 +193,7 @@ function getStandardName(name) {
if (SVGDOM_ATTRIBUTE_NAMES[name]) {
return SVGDOM_ATTRIBUTE_NAMES[name];
}
let i;
let i = -1;

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

This seems to be useless, but I assume tells tsc what the type of i is. It could also be set to 0, but -1 is clearer semantically.

This comment has been minimized.

Copy link
@golopot

golopot May 12, 2019

Author Contributor

If i is not initialized, it could potentially be undefined, that would make tsc reject DOM_PROPERTY_NAMES[i].

This comment has been minimized.

Copy link
@ljharb

ljharb May 13, 2019

Collaborator

[undefined] on an object without that property is safely undefined, that's just the type system being overly strict :-)

Show resolved Hide resolved lib/rules/no-unused-state.js Outdated
*/
function reportNodeIncorrectlyPositioned(node, expectedRule) {
// Detect if this node is an expected property declaration adn return the property name
const name = classProperties.find(propertyName => {
if (propertiesToCheck[propertyName](node)) {
return propertyName;
return !!propertyName;

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

Assuming the return value of reportNodeIncorrectlyPositioned is always used in a truthy/falsy position, this just returns true instead of "truthy"…

}

return null;
return false;

This comment has been minimized.

Copy link
@ljharb

ljharb May 12, 2019

Collaborator

and false instead of "falsy".

Show resolved Hide resolved lib/types.d.ts Outdated
@golopot

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Thanks for the thorough review!

@ljharb ljharb force-pushed the golopot:fix-some-jsdoc-errors branch 2 times, most recently from e6509bb to 3e7c1f6 May 14, 2019

@ljharb ljharb merged commit a659b63 into yannickcr:master May 14, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 97.452%
Details

@golopot golopot deleted the golopot:fix-some-jsdoc-errors branch May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.