Update index.d.ts for react-redux #20848
Conversation
known issue described in comments
|
types/react-redux/index.d.ts to authors (@tkqubo @thasner @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo). Could you review this PR? |
typescript-bot
added
the
Popular package
types/react-redux/index.d.ts
Outdated
| @@ -12,7 +12,7 @@ | |||
| // TypeScript Version: 2.4 | |||
| // There is a known issue in TypeScript, which doesnt allow decorators to change the signature of the classes | |||
There was a problem hiding this comment.
Should be "doesn't", not "doesnt".
types/react-redux/index.d.ts
Outdated
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
| // TypeScript Version: 2.4 | ||
|
|
||
| // There is a known issue in TypeScript, which doesnt allow decorators to change the signature of the classes |
There was a problem hiding this comment.
I'm not sure if this comment should be part of the rest of this "block" -- could you move it down to where the connect function is declared?
There was a problem hiding this comment.
its here so its obvious to the person using this type definition.
react-redux is eseentially just the connect() function. as soon as someone use this definition, they will be bombarded with a bunch of errors. so better to put this at the very top instead of hoping they will hunt and find this message.
There was a problem hiding this comment.
Ok, then can you at least separate it from the version comment by a newline? I'd like to see it as its own thing, rather than visually blending in.
There was a problem hiding this comment.
@pdeva not required for merge, but it would be nice to have some mention of this issue down where the connect function is declared as well. If someone is using ts-server or an IDE, they will be taken straight to the definition, not the top of the file. Something like "See file header for a known issue about using the connect function as a decorator." would work.
|
@pdeva Left a response on one of kenzierocks's comments above. |
|
Approved by a listed owner. PR ready to merge pending express review by a maintainer. |
Please fill in this template.
known issue described in comments
Select one of these and delete the others:
If changing an existing definition:
#20796