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.

Sign up for GitHub

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
Merged

chore(url): add jsdoc typescript checking #17014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe781fc
chore(url): add jsdoc typescript checking
dsifford Aug 12, 2019
28d8a01
add typescript checks to lint job
dsifford Aug 21, 2019
a283b6f
add typescript as devDependency
dsifford Aug 21, 2019
fa0d540
fix(url): improve type annotation for clarity
dsifford Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@
"source-map-loader": "0.2.4",
"sprintf-js": "1.1.1",
"stylelint-config-wordpress": "13.1.0",
"typescript": "3.5.3",
"uuid": "3.3.2",
"webpack": "4.8.3",
"worker-farm": "1.7.0"
Expand Down Expand Up @@ -189,13 +190,14 @@
"fixtures:server-registered": "docker-compose run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > test/integration/full-content/server-registered.json",
"fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit",
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate",
"lint": "concurrently \"npm run lint-js\" \"npm run lint-pkg-json\" \"npm run lint-css\"",
"lint": "concurrently \"npm run lint-js\" \"npm run lint-pkg-json\" \"npm run lint-css\" \"npm run lint-types\"",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry about that. On it now.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

"package-plugin": "./bin/build-plugin-zip.sh",
"pot-to-php": "./bin/pot-to-php.js",
"precommit": "lint-staged",
Expand Down
32 changes: 16 additions & 16 deletions packages/url/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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" ].

Copy link
Contributor Author

@dsifford dsifford Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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)

- _args_ `Object`: Query arguments to apply to URL.

_Returns_
Expand Down Expand Up @@ -72,7 +72,7 @@ _Parameters_

_Returns_

- `?string`: The authority part of the URL.
- `(string|void)`: The authority part of the URL.

<a name="getFragment" href="#getFragment">#</a> **getFragment**

Expand All @@ -91,7 +91,7 @@ _Parameters_

_Returns_

- `?string`: The fragment part of the URL.
- `(string|void)`: The fragment part of the URL.

<a name="getPath" href="#getPath">#</a> **getPath**

Expand All @@ -110,7 +110,7 @@ _Parameters_

_Returns_

- `?string`: The path part of the URL.
- `(string|void)`: The path part of the URL.

<a name="getProtocol" href="#getProtocol">#</a> **getProtocol**

Expand All @@ -129,7 +129,7 @@ _Parameters_

_Returns_

- `?string`: The protocol part of the URL.
- `(string|void)`: The protocol part of the URL.

<a name="getQueryArg" href="#getQueryArg">#</a> **getQueryArg**

Expand All @@ -143,12 +143,12 @@ const foo = getQueryArg( 'https://wordpress.org?foo=bar&bar=baz', 'foo' ); // ba

_Parameters_

- _url_ `string`: URL
- _arg_ `string`: Query arg name
- _url_ `string`: URL.
- _arg_ `string`: Query arg name.

_Returns_

- `(Array|string)`: Query arg value.
- `(QueryArgParsed|undefined)`: Query arg value.

<a name="getQueryString" href="#getQueryString">#</a> **getQueryString**

Expand All @@ -167,7 +167,7 @@ _Parameters_

_Returns_

- `?string`: The query string part of the URL.
- `(string|void)`: The query string part of the URL.

<a name="hasQueryArg" href="#hasQueryArg">#</a> **hasQueryArg**

Expand All @@ -181,8 +181,8 @@ const hasBar = hasQueryArg( 'https://wordpress.org?foo=bar&bar=baz', 'bar' ); //

_Parameters_

- _url_ `string`: URL
- _arg_ `string`: Query arg name
- _url_ `string`: URL.
- _arg_ `string`: Query arg name.

_Returns_

Expand Down Expand Up @@ -331,11 +331,11 @@ const actualURL = prependHTTP( 'wordpress.org' ); // http://wordpress.org

_Parameters_

- _url_ `string`: The URL to test
- _url_ `string`: The URL to test.

_Returns_

- `string`: The updated URL
- `string`: The updated URL.

<a name="removeQueryArgs" href="#removeQueryArgs">#</a> **removeQueryArgs**

Expand All @@ -349,12 +349,12 @@ const newUrl = removeQueryArgs( 'https://wordpress.org?foo=bar&bar=baz&baz=fooba

_Parameters_

- _url_ `string`: URL
- _args_ `...string`: Query Args
- _url_ `string`: URL.
- _args_ `...string`: Query Args.

_Returns_

- `string`: Updated URL
- `string`: Updated URL.

<a name="safeDecodeURI" href="#safeDecodeURI">#</a> **safeDecodeURI**

Expand Down
44 changes: 26 additions & 18 deletions packages/url/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ const URL_REGEXP = /^(?:https?:)?\/\/\S+$/i;
const EMAIL_REGEXP = /^(mailto:)?[a-z0-9._%+-]+@[a-z0-9][a-z0-9.-]*\.[a-z]{2,63}$/i;
const USABLE_HREF_REGEXP = /^(?:[a-z]+:|#|\?|\.|\/)/i;

/**
* @typedef {{[key: string]: QueryArgParsed}} QueryArgObject
*/

/**
* @typedef {string|string[]|QueryArgObject} QueryArgParsed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

*/

/**
* Determines whether the given string looks like a URL.
*
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@dsifford dsifford Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

Copy link
Member

Choose a reason for hiding this comment

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

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.

*/
export function getProtocol( url ) {
const matches = /^([^\s:]+:)/.exec( url );
Expand Down Expand Up @@ -90,7 +98,7 @@ export function isValidProtocol( protocol ) {
* const authority2 = getAuthority( 'https://localhost:8080/test/' ); // 'localhost:8080'
* ```
*
* @return {?string} The authority part of the URL.
* @return {string|void} The authority part of the URL.
*/
export function getAuthority( url ) {
const matches = /^[^\/\s:]+:(?:\/\/)?\/?([^\/\s#?]+)[\/#?]{0,1}\S*$/.exec( url );
Expand Down Expand Up @@ -130,7 +138,7 @@ export function isValidAuthority( authority ) {
* const path2 = getPath( 'https://wordpress.org/help/faq/' ); // 'help/faq'
* ```
*
* @return {?string} The path part of the URL.
* @return {string|void} The path part of the URL.
*/
export function getPath( url ) {
const matches = /^[^\/\s:]+:(?:\/\/)?[^\/\s#?]+[\/]([^\s#?]+)[#?]{0,1}\S*$/.exec( url );
Expand Down Expand Up @@ -170,7 +178,7 @@ export function isValidPath( path ) {
* const queryString2 = getQueryString( 'https://wordpress.org#fragment?query=false&search=hello' ); // 'query=false&search=hello'
* ```
*
* @return {?string} The query string part of the URL.
* @return {string|void} The query string part of the URL.
*/
export function getQueryString( url ) {
const matches = /^\S+?\?([^\s#]+)/.exec( url );
Expand Down Expand Up @@ -210,7 +218,7 @@ export function isValidQueryString( queryString ) {
* const fragment2 = getFragment( 'https://wordpress.org#another-fragment?query=true' ); // '#another-fragment'
* ```
*
* @return {?string} The fragment part of the URL.
* @return {string|void} The fragment part of the URL.
*/
export function getFragment( url ) {
const matches = /^\S+?(#[^\s\?]*)/.exec( url );
Expand Down Expand Up @@ -244,9 +252,9 @@ export function isValidFragment( fragment ) {
* includes query arguments, the arguments are merged with (and take precedent
* over) the existing set.
*
* @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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

* only the resulting querystring is returned.
* @param {Object} args Query arguments to apply to URL.
*
* @example
* ```js
Expand Down Expand Up @@ -282,15 +290,15 @@ export function addQueryArgs( url = '', args ) {
/**
* Returns a single query argument of the url
*
* @param {string} url URL
* @param {string} arg Query arg name
* @param {string} url URL.
* @param {string} arg Query arg name.
*
* @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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note QueryArgParsed is used here.

Copy link
Member

Choose a reason for hiding this comment

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

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.

*/
export function getQueryArg( url, arg ) {
const queryStringIndex = url.indexOf( '?' );
Expand All @@ -302,8 +310,8 @@ export function getQueryArg( url, arg ) {
/**
* Determines whether the URL contains a given query arg.
*
* @param {string} url URL
* @param {string} arg Query arg name
* @param {string} url URL.
* @param {string} arg Query arg name.
*
* @example
* ```js
Expand All @@ -319,15 +327,15 @@ export function hasQueryArg( url, arg ) {
/**
* Removes arguments from the query string of the url
*
* @param {string} url URL
* @param {...string} args Query Args
* @param {string} url URL.
* @param {...string} args Query Args.
*
* @example
* ```js
* const newUrl = removeQueryArgs( 'https://wordpress.org?foo=bar&bar=baz&baz=foobar', 'foo', 'bar' ); // https://wordpress.org?baz=foobar
* ```
*
* @return {string} Updated URL
* @return {string} Updated URL.
*/
export function removeQueryArgs( url, ...args ) {
const queryStringIndex = url.indexOf( '?' );
Expand All @@ -342,14 +350,14 @@ export function removeQueryArgs( url, ...args ) {
/**
* Prepends "http://" to a url, if it looks like something that is meant to be a TLD.
*
* @param {string} url The URL to test
* @param {string} url The URL to test.
*
* @example
* ```js
* const actualURL = prependHTTP( 'wordpress.org' ); // http://wordpress.org
* ```
*
* @return {string} The updated URL
* @return {string} The updated URL.
*/
export function prependHTTP( url ) {
if ( ! USABLE_HREF_REGEXP.test( url ) && ! EMAIL_REGEXP.test( url ) ) {
Expand Down
32 changes: 32 additions & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"compilerOptions": {
"allowJs": true,
"allowSyntheticDefaultImports": true,
"checkJs": true,
"jsx": "preserve",
"lib": ["dom", "esnext"],
"module": "commonjs",
"noEmit": true,
"resolveJsonModule": true,
"target": "esnext",

/* 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. */
Copy link
Member

Choose a reason for hiding this comment

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

Why do we allow implicit any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@dsifford dsifford Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍


/* Additional Checks */
"noUnusedLocals": true, /* Report errors on unused locals. */
"noUnusedParameters": true, /* Report errors on unused parameters. */
"noImplicitReturns": true, /* Report error when not all code paths in function return a value. */
"noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */

},
"include": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

At some point, you will have to include:

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"./packages/**/build/**",
"./packages/**/build-module/**"
]
}