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

Improved checkJS #285

Open
wants to merge 3 commits into
base: ts
Choose a base branch
from
Open

Improved checkJS #285

wants to merge 3 commits into from

Commits on Oct 3, 2018

  1. Update to TS@next and fix resulting type errors

    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
    sandersn committed Oct 3, 2018
    Configuration menu
    Copy the full SHA
    511942a View commit details
    Browse the repository at this point in the history
  2. Remove d.ts files

    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.
    sandersn committed Oct 3, 2018
    Configuration menu
    Copy the full SHA
    1b10cec View commit details
    Browse the repository at this point in the history

Commits on Oct 4, 2018

  1. Move file-specific types back to JSDoc

    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.)
    sandersn committed Oct 4, 2018
    Configuration menu
    Copy the full SHA
    cc45aac View commit details
    Browse the repository at this point in the history