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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added types. #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added types. #20

wants to merge 2 commits into from

Conversation

loucyx
Copy link

@loucyx loucyx commented Nov 20, 2018

I'm working a lot with TypeScript+Preact+Stockroom, and I needed types to make my workflow even better. Let's make everyone's worflow better with TS 馃槃

developit
developit reviewed Mar 8, 2019
View reviewed changes
worker.d.ts
* Retrieves the given action.
* @param action Action to retrieve (Worker actions take strings).
*/
action(action: Action<WorkerState> | string): (...params: any[]) => void;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this one is already defined on Store, right?

Copy link
Owner

Choose a reason for hiding this comment

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

https://github.com/developit/unistore/blob/master/index.d.ts#L12

Copy link
Author

Choose a reason for hiding this comment

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

Hi @developit, no worries about the time to review (with the recent release of Preact X, I understand). About your question, Unistore's action doesn't work exactly like Stockroom's action. Stockroom takes an Action or a string, and Unistore (afaik) only takes an Action. Also Stockroom returns void where Unistore returns the BoundAction.

developit
developit approved these changes Mar 8, 2019
View reviewed changes
Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

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

Sorry for the super slow review, these look great! Just the one question about Store.action().

@developit developit added the enhancement New feature or request label Mar 8, 2019
@loucyx
Copy link
Author

loucyx commented Mar 8, 2019 •
edited

This should solve both #22 and #21. To my credit compared to #22, I used JSDocs with the definitions to make them more clear 馃槃

@Silic0nS0ldier
Copy link

I'm all for having the JSDoc in the definitions. Had a bit of a debate when I was putting the types together for #22 and ultimately figured it would add to maintenance time (double handling of function docs).

馃 I know that the TypeScript compiler is capable of linting regular JS via JSDoc. I wonder if definition files could be generated from that... Something I think I'll look into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@developit developit developit approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@loucyx @Silic0nS0ldier @developit