Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

Improve typescript typing #363

Merged
merged 2 commits into from
Mar 6, 2019
Merged

Improve typescript typing #363

merged 2 commits into from
Mar 6, 2019

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jan 24, 2019

Problem

With typescript, the normalize object lose his type

Solution

Generic function

TODO

  • Add & update tests
  • Ensure CI is passing (lint, tests, flow)
  • Update relevant documentation

@VincentLanglet VincentLanglet changed the title Update index.d.ts Improve typescript typing Feb 16, 2019
@@ -49,13 +49,12 @@ export type Schema =
schema.Values[] |
{[key: string]: Schema | Schema[]};

export function normalize(
export type NormalizedSchema<E, R> = { entities: E, result: R };
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want entities to be something like { [key:string]: { [key:string]: E } } since it's guaranteed to be nested with string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this example, the entities are not the same

{
  result: "123",
  entities: {
    "articles": {
      "123": {
        id: "123",
        author: "1",
        title: "My awesome blog post",
        comments: [ "324" ]
      }
    },
    "users": {
      "1": { "id": "1", "name": "Paul" },
      "2": { "id": "2", "name": "Nicole" }
    },
    "comments": {
      "324": { id: "324", "commenter": "2" }
    }
  }
}

You prefer to type this, like this

type List<E> = {
  [key: string]: E
}

type Entities = {
  articles: List<Article>
  users: List<User>
  comments: List<Comment>
}

And then NormalizedSchema<Entities>

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see how that would be more specific if you put the work in. For my cases I was happy with 'any' for the final result and really wanted to have the typings of the nesting level.

For me there's no way to know all the entity types statically, so this is unfeasible for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I let the default any value so you can use it like you do.

But this can't be use with the no-unsafe-any rule from tslint.

It's not about typescript checking the type of the entity. It about typescript checking the use of the entity after you normalized them.

cont normalized = normalize<Entities>(...);

const foo = normalized.articles // known as Article.
foo.author // no error
foo.name // error
const bar = normalized.user // error

With the less precise typing you ask, we will have

const foo = normalized.articles // known as Entity = Article | User | Comment
foo.author // error !!!
foo.name // error
const bar = normalized.user // no error !!!

Choose a reason for hiding this comment

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

this makes sense, the typing should be on the entity. also @ntucker you can opt out by using any.

Copy link
Owner

@paularmstrong paularmstrong left a comment

Choose a reason for hiding this comment

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

Thanks! The addition of generics really helps avoid default any all over.

@paularmstrong paularmstrong merged commit 99acce3 into paularmstrong:master Mar 6, 2019
@lock
Copy link

lock bot commented Sep 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the Outdated label Sep 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants