-
Notifications
You must be signed in to change notification settings - Fork 868
Conversation
@blakeembrey could you please review? |
@unindented Just remember that TypeScript is going to do structural type checking, so some of those values already appear to be assignable to each other. |
@@ -0,0 +1,67 @@ | |||
interface EntitySchemaOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will probably want to export these interfaces/types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All types that are used by public APIs should be exported? So I should export everything in this file, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much, export everything.
@blakeembrey Forgive my ignorance, I'm very new to type systems. Could you give me an example to illustrate the fact that "some of those values already appear to be assignable to each other", and what consequences it has? |
@gaearon What are your thoughts on adding type definitions to the repo? I have an attempt at TypeScript definitions here, and could work on a |
@unindented This is the basic gist: http://www.typescriptlang.org/play/index.html#src=class%20Foo%20%7B%7D%0A%0Aclass%20Bar%20%7B%7D%0A%0Afunction%20x%20(foo%3A%20Foo)%20%7B%7D%0A%0Ax(new%20Bar())%0Ax('test')%0Ax(10)%0Ax(Foo) Since all the interfaces are structurally the same, you aren't actually doing any type checking. |
@blakeembrey Aha, understood. Adding empty interfaces is almost equivalent to I've updated the PR with proper types. |
@paularmstrong @gaearon do you think the type definitions belong to this repo? There are ways to host the type definitions in a separate repo, but then we wouldn't be able to reference them in the |
I don't see why normalizr shouldn't include its own type declarations. However, is there a way to have an automated test ensure that these are updated when necessary? |
Umm, I don't know the best way to ensure type declarations are up-to-date. @blakeembrey what do you suggest? |
If we add them, we should add tests for them. reduxjs/redux#1413 These include tests. |
Ok, I'll follow the same approach. |
Yes, that's what we usually do too. You can have some valid code and make sure it compiles validly with |
Hey @paularmstrong, I finally got around to this. I've updated the definitions with the latest changes that were made ( Let me know what you think! |
Thanks @unindented. I'll merge this soon for v2.2.0 |
@paularmstrong any planned date for |
Been on vacation. Maybe can shoot for late next week. If there's a strong request, though, I could defer the docs/examples/tests updates that we need. |
Tests are already in this PR. Other projects, like |
@unindented I mean #105, #106, and #107 |
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. |
No description provided.