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(url): add jsdoc typescript checking #17014

Merged
merged 4 commits into from Aug 28, 2019

Conversation

@dsifford
Copy link
Contributor

commented Aug 12, 2019

Description

This is the first of many PRs that seek to introduce JSDoc TypeScript type checking to packages. I chose this package as the first one because it's one of the easier ones to do and should be a good starting point for reviewers to familiarize themselves.

I'll make inline comments to some of the changes that I think are most notable. Otherwise, this should be fairly straightforward.

One final thing to note: I decided to add in a tsconfig.json file alongside the existing jsconfig.json file because the configurations as of right this moment are uniquely different and serve different purposes. Once all the packages have been updated with better JSDoc, we can either keep both of these files OR delete the tsconfig.json and merge in the changes into the jsconfig.json, whatever you all prefer.

How has this been tested?

All tests still passing.

Screenshots

Types of changes

Code quality / Documentation.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@dsifford dsifford requested a review from talldan as a code owner Aug 12, 2019
Copy link
Contributor Author

left a comment

Final note: a bunch of the checks are failing for whatever reason. Those are unrelated to the changes I made here as far as I can tell.

*/

/**
* @typedef {string|string[]|QueryArgObject} QueryArgParsed

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 12, 2019

Author Contributor

This was added to more closely represent the possible return types of getQueryArg.

It appears that it is currently possible for the response to be an object, containing keys whose values are either string or string[] or simply a string or string[] by itself.

This definition accounts for that. There needs to be two separate definitions because they are both cyclic and recursive.

*
* @example
* ```js
* const foo = getQueryArg( 'https://wordpress.org?foo=bar&bar=baz', 'foo' ); // bar
* ```
*
* @return {Array|string} Query arg value.
* @return {QueryArgParsed|undefined} Query arg value.

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 12, 2019

Author Contributor

Note QueryArgParsed is used here.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I thought we might leverage qs types here, but parse is typed as:

function parse(str: string, options?: IParseOptions): any;

That's not very helpful, I suspect it's very difficult to type the return because it can vary based on the input and options. We control the options and can likely provide better type information as you've done.

"noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */

},
"include": [

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 12, 2019

Author Contributor

The include array will be changed with each new package that is added. We're skipping tests because almost universally throughout the project, incorrect partial types are fed to functions to test a specific case which is not allowed as far as TypeScript is concerned. Fixing all of those instances would require a pretty significant amount of work and there's no real benefit to doing it.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 28, 2019

Member

Yes, let's skip tests for now and revisit our approach once we have all packages enabled here 👍

* @param {?string} url URL to which arguments should be appended. If omitted,
* only the resulting querystring is returned.
* @param {Object} args Query arguments to apply to URL.
* @param {string} [url=''] URL to which arguments should be appended. If omitted,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 13, 2019

Contributor

The default value seems like a duplication for me, can't it be infered?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 13, 2019

Author Contributor

This is mostly important for documentation generation. Otherwise, I'm mainly just following the WordPress JSDoc standards posted here.

To answer your question: Yes, the TypeScript compiler can infer this.

Copy link
Contributor

left a comment

How can we "use" these checks? Can we add a CI job to check the types or add it to the lint job?

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

How can we "use" these checks? Can we add a CI job to check the types or add it to the lint job?

Yes, but I was under the impression that it was preferred that I hold off on doing that until the rest of the packages are finished. If not, I can add it into the CI flow no problem.

To run the TypeScript compiler locally yourself, it's as easy as running npx tsc from anywhere in the project.


Speaking of checks, what's going on with the checks for this PR? Lots of actions are failing.

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Ping @youknowriad -- See comments above. Is this good to merge?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Yes, but I was under the impression that it was preferred that I hold off on doing that until the rest of the packages are finished. If not, I can add it into the CI flow no problem.

Can we try this, fees like we can add this pretty easily to the lint job right?

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@youknowriad Done.

"lint-js": "wp-scripts lint-js",
"lint-js:fix": "npm run lint-js -- --fix",
"lint-php": "docker-compose run --rm composer run-script lint",
"lint-pkg-json": "wp-scripts lint-pkg-json ./packages",
"lint-css": "wp-scripts lint-style '**/*.scss'",
"lint-css:fix": "npm run lint-css -- --fix",
"lint-types": "tsc",

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 21, 2019

Contributor

Should we install the typescript dev dependency? I think travis is failing because of it?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 21, 2019

Author Contributor

Whoops, sorry about that. On it now.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 27, 2019

Member

Should we include this command in lint-staged section of this file to have it executed as a pre-commit hook?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

Not sure if we can lint only a single file at a time because type compilation generally requires knowledge of the entire project.

Also, the compiler seems to freak out if you feed it files with a .js extension by itself (regardless of whether or not they are allowed)...

$ tsc packages/autop/src/index.js
error TS6054: File 'packages/autop/src/index.js' has unsupported extension. The only supported extensions are '.ts', '.tsx', '.d.ts'.


Found 1 error.

So, in short, we can add it it the lint-staged flow, but it technically won't be able to lint just the staged file(s). Up to you.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 27, 2019

Member

We can skip it for now if it produces issues, no worries. As long as Travis is going to catch those violations, we are good 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

There's one last failing, I think you should probably just rebase the branch for it to pass.

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Re: The failing step --- I think this is unrelated to this PR specifically since that same check was failing from all PRs since (IIRC) 2 days before this PR until (at least) 2 days ago.

Do you still want me to rebase anyway? If so, I'll do it on Monday. I'll be away all weekend.

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Do you still want me to rebase anyway? If so, I'll do it on Monday. I'll be away all weekend.

Yes, it would be great to rebase. Riad is off for the next two weeks, but I can help to land it :)

Copy link
Member

left a comment

This is great, I'm excited to start leveraging all your work around TypeScript. I've left some notes and questions on the PR.

One final thing to note: I decided to add in a tsconfig.json file alongside the existing jsconfig.json file because the configurations as of right this moment are uniquely different and serve different purposes. Once all the packages have been updated with better JSDoc, we can either keep both of these files OR delete the tsconfig.json and merge in the changes into the jsconfig.json, whatever you all prefer.

This note leaves me with additional questions, can you elaborate a bit more? Particularly, it's unclear yo me how they can be "uniquely different and serve different purposes", but in the future one of them would become redundant.


/* 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. */

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

Why do we allow implicit any?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

We can decide to get stricter in the future, but adding noImplicitAny as it stands right now would require a lot of variable annotations in the code. Goal is to make the initial transition as seamless and painless as possible.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I tried disabling this and the only error was missing types for qs module. Do you expect we'll start pulling in type declarations as devDependencies?

I'm on the fence because it seems much easier to relax things when you're too strict than to become more strict when you've been too relaxed 🙂

Have you already investigated parts of the code where strictness here becomes overly cumbersome and could you provide an example?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

I have not. But I'm nervous that we'd lose people who bought into this crazy idea if I told them we'd have to depend on more external types.

No specific examples I can think of off the top of my head but, as you've already alluded, the issues that arise will be almost (if not entirely) exclusive to missing type definitions for externals.

We could fix that if we pull in those defs from DefinitelyTyped, however there was some concern already about pulling in the types for this repo from DefinitelyTyped so not sure how the response to that ask would go over (see side note).

I'm all for making it as strict as possible.


Side note: Why would we be pulling in externals for this repo when we have perfectly good JSDoc here already? The answer to that is that although JSDoc provides fairly good types in comparison to native typescript, it does not allow for the inference that native typescript can produce with respect to generics or overloaded functions (yet). Also, the DefinitelyTyped definitions have all the React components strongly typed as well.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I'm nervous that we'd lose people who bought into this crazy idea if I told them we'd have to depend on more external types.

Good point 👍

packages/url/src/index.js Outdated Show resolved Hide resolved
*
* @example
* ```js
* const foo = getQueryArg( 'https://wordpress.org?foo=bar&bar=baz', 'foo' ); // bar
* ```
*
* @return {Array|string} Query arg value.
* @return {QueryArgParsed|undefined} Query arg value.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I thought we might leverage qs types here, but parse is typed as:

function parse(str: string, options?: IParseOptions): any;

That's not very helpful, I suspect it's very difficult to type the return because it can vary based on the input and options. We control the options and can likely provide better type information as you've done.

@@ -30,7 +30,7 @@ const newURL = addQueryArgs( 'https://google.com', { q: 'test' } ); // https://g

_Parameters_

- _url_ `?string`: URL to which arguments should be appended. If omitted, only the resulting querystring is returned.
- _url_ `[string]`: URL to which arguments should be appended. If omitted, only the resulting querystring is returned.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I find the [string] syntax strange. Where does it come from?

I assume it means "optional parameter with a default value", but that only seems to vaguely align with jsdoc, where [square brackets] are used on the parameter or property names to indicate they're optional, not around the type.

To add to the confusion, in TypeScript [ string ] would mean a one-tuple of strings, or an array with a single string member, e.g. [ "foo" ] but never [] or [ "foo", "bar" ].

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

Yeah, as it stands, the way that the JSDoc is being parsed definitely results in clunky markdown for this particular case.

In JSDoc, it's pretty easy to tell the difference between optional types and single-tuple types because they are written differently.

// optional string type
/**
 * @param {string} [foo]
 */

// single tuple string type
/**
 * @param {[string]} foo
 */

So I think that you've definitely raised a valid concern with respect to how the docs are being generated.

(PS: syntax comes from here)

@@ -50,7 +58,7 @@ export function isEmail( email ) {
* const protocol2 = getProtocol( 'https://wordpress.org' ); // 'https:'
* ```
*
* @return {?string} The protocol part of the URL.
* @return {string|void} The protocol part of the URL.

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

Regarding the return types here in general, it's been bothering me a bit that some are SomeType | void and another is SomeType | undefined

It appears that type void = undefined | null or with strictNullChecks it's type void = undefined (our case). However, when I think of functions that return void, I tend to think of side-effects. Using void in a union with another type to indicate we may or may not get a value seems like something better expressed by SomeType | undefined.

As far as I can tell and for our purposes, void and undefined are type aliases, so this is very much a stylistic preference, but is this something you considered and do you have thoughts on it?

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

This is a good observation and for all intents and purposes void and undefined are functionally equivalent when strict checking is enabled.

....however.

The TypeScript parser treats the following functions differently...

// returns `void`
function foo() {
  return;
}

// returns `undefined`
function bar() {
  return undefined;
}

// returns `void`
function baz() {}

// returns `number | void` *see note
function qux(thing) {
  if (thing) {
    return 123;
  }
}

Actually, when writing those test cases above, I threw them at the typescript compiler just to make sure I wasn't mistaken and the last case technically returns number | undefined. I think this is because it does sometimes return a value, the void is just turned into undefined.

So, for the case you point out, you're right. This should be string|undefined here. 👍

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 27, 2019

Author Contributor

Ah, this is why I did it the way I did it....

image

This comment has been minimized.

Copy link
@sirreal

sirreal Aug 27, 2019

Member

I noticed that in the TypeScript playground, a return statement —even empty return;— is required to return undefined instead of void. Viewed from this perspective and accustomed to JavaScript's implicit undefined return, that seems like a strange decision 🤷‍♂

It's tempting to go add empty returns to the functions that are returning string | void, but TypeScript should be supporting us and not the other way around.

These are fine to leave, thanks for your thoughts 👍

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 28, 2019

Member

My only worry is that we will have to update all occurrences where the return value is optional. It might be quite a huge number of lines. For what it's worth, it's at list very explicit.

Co-Authored-By: Jon Surrell <jon.surrell@automattic.com>
@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@sirreal

This note leaves me with additional questions, can you elaborate a bit more? Particularly, it's unclear yo me how they can be "uniquely different and serve different purposes", but in the future one of them would become redundant.

Sorry for the confusion. Right now the jsconfig's role is to provide type hints where able in editors that support it for all packages.

Conversely, the tsconfig.json is providing type hints and also compiling for errors in only the specific files that we include in the include section (gradually adding more and more packages as they are fixed). Once all the types are fixed and it compiles without issues, the two config files will essentially converge and one of them will be redundant.

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@gziolo Has this already been rebased? The travis build is passing now and it appears that these commits are ahead of the commits on master (I had to delete my local repo and re-pull to make sure I was seeing this correctly).

Copy link
Member

left a comment

This looks great to me, thanks @dsifford 🎉

There's one improvement I'd like to see for types in the generated documentation, but that's not introduced in this PR and can be addressed later.

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

There's one improvement I'd like to see for types in the generated documentation, but that's not introduced in this PR and can be addressed later.

It's your call whether you want to include it in this PR or do you plan to work on a follow-up. You should be able to merge yourself if you pick the first option. I trust @sirreal more than myself in the context of TypeScript 😅

"./packages/url/**/*.js"
],
"exclude": [
"./packages/**/test/**",

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 28, 2019

Member

At some point, you will have to include:

  • "./packages/**/benchmark/**",

Just saying, there are a few packages which use them :)

This comment has been minimized.

Copy link
@dsifford

dsifford Aug 28, 2019

Author Contributor

Good point. I'll add it in when the issue arises if that's okay with you.

@dsifford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@gziolo

It's your call whether you want to include it in this PR or do you plan to work on a follow-up. You should be able to merge yourself if you pick the first option. I trust @sirreal more than myself in the context of TypeScript

We can try to get something together in this PR if you'd prefer that, but it might require a bit of work and some design input from the team to get done properly. The changes would have to occur in the package that generates the markdown documentation.

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

We can try to get something together in this PR if you'd prefer that, but it might require a bit of work and some design input from the team to get done properly. The changes would have to occur in the package that generates the markdown documentation.

I don't think I can help this week, so it's totally fine to open an issue and merge it in the current shape.

@dsifford dsifford merged commit f7e067b into master Aug 28, 2019
1 of 4 checks passed
1 of 4 checks passed
Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@dsifford dsifford deleted the add/url/typescript-jsdoc-checking branch Aug 28, 2019
@gziolo gziolo added this to the Gutenberg 6.5 milestone Aug 30, 2019
@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

I filed a follow-up #17271 to keep track of issues raised in #17014 (review) by @sirreal.

dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* chore(url): add jsdoc typescript checking

* add typescript checks to lint job

* add typescript as devDependency

* fix(url): improve type annotation for clarity

Co-Authored-By: Jon Surrell <jon.surrell@automattic.com>
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.