Improved checkJS #285
Open
Improved checkJS #285
+215
−225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also simplify and modify type aliases where necessary.
Note that I haven't fixed any implicit any errors. I also kept the
balance between d.ts and jsdoc about the same.
In lib/formats.ts, stricter call is unable to handle the multiple
overloads of String.prototype.replace, so I had to manually type it to
the one I wanted.
In lib/q.d.ts, ObjectCoercible and ObjectIndexable were effectively the
same, so I removed ObjectIndexable. Also, `{}` subsumes primitive, so I
changed it to `object`:
```ts
type ObjectCoercible = object | NonNullishPrimitive;
```
I changed some similar types that were almost right:
`Concatable<object>` needs to be `Concatable<{}>` because of the way
it's used in utility.compact, and `object | Primitive` is the same as
`unknown`, so I changed `Value` to `Concatable<unknown>`.
In lib/utils.js, I had to edit `merge` in a few places. At least a
couple of the edits looked like they were fixing potential bugs, but I
had trouble visualising the execution of `merge` so I can't be sure.
1. When `typeof source !== "object"`, it could still be `"boolean"`, but
booleans can't be used to index into Object.prototype, so I added an
additional guard.
2. `typeof target !== "object"` is always false, so I deleted the branch
that used it; the code inside was a type error because the compiler
thinks it can't be reached. I couldn't convince myself that it was
wrong.
3. Narrowing by element access failed, so I had to create a `const tmp =
target[i]` before checking that `typeof tmp === "object"`.
4. In `compact`, and I had to add a type annotation to `queue` and then
a cast when pushing into it. The former isn't that surprising,
considering that it's supposed to have a complex type that we can't
infer. The latter is a result of checks that prove that the object
structure has a recursive level that needs to be compacted, but the
compiler can't follow the checks and needs to be told explicitly about
the type.
In tsconfig.json, I had to manually set `"target": "esnext"` to avoid
some bogus errors.
Breakdown of the changes:
* 4 cases where the compiler failed and needed a workaround
* 2 possible bugs
* 3 cases where the type system is full of confusing, similar types
Note that qs.d.ts remains. The other d.ts files were actually confusing
the compiler's module resolution such that `var x = require('./x')`
would assign the type `any` to `x` when both `x.js` and `x.d.ts`
existed.
With better typings, I had to fix a number of discrepancies, mainly in
the types:
In parse.js, the options initialisation pattern is not well-understood
by the compiler and requires a type annotation for the initial
assignment, then an temp variable with a cast once all the values have
been filled in:
```js
var internalOptions = /** @type {ParseOptionsInternal} */(options);
```
The Typescript-like pattern would be more like:
```js
/** @type {ParseOptions} */
var options = opts ? utils.assign({}, opts) : {};
var internalOptions = {
ignoreQueryPrefix: options.ignoreQueryPrefix === true,
// ...
}
```
In q.d.ts, I had to unsimplify
`type Value = Concatable<object | Primitive>`. Turns out `unknown`
doesn't narrow correctly.
util.compact's type is actually `object => object` not `ObjectCoercible
=> ObjectCoercible`, since it doesn't appear to ever return a primitive.
The rest of the type changes were just correcting discrepencies in the
options types since the checker wasn't really checking those because of
the duplicate d.ts files.
In stringify, the filter can be an array of string or number. However,
in this case, elements of the filter array end up used in places where
only a string is expected. I inserted a couple of conversions to string
where required.
In the parse tests, a few tests rely on the fact that a previous test
asserts that a field exists, but the type system doesn't know this to be
true. I just annotated these instances with `any` because the type
checking isn't adding value here.
Global aliases like Nullish are now global so they don't need to be imported. I also introduced `type NonPrimitive = object` to work around the way Typescript treats `object` as `any` in JS. The move required hardly any changes; in fact, I basically restored the annotations from commit 856582e with updates for the fixes in my previous commits. Introducing NonPrimitive required a few changes since `object` is stricter than `any`. The most annoying is a cast on ```js /** @type {any} */([]).concat(leaf); ``` Because with strictNullChecks on, the type of `[]` is `never[]`, which is technically correct but usually annoying. (strictNullChecks is usually not a good fit for JS codebases that don't want to adopt Typescript idioms completely, and `[].concat` is the best example of this.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves most types back to JSDoc and fixes a few remaining type errors. Each commit has a discussion of what changed, and the commits are organised so that their diffs should individually be readable. I'm happy to discuss the hows and whys, since I want to understand what it's like to adopt checkJS for a JS expert.