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

docs(typescript): clean up bounded useStore recipe #1581

Merged
merged 4 commits into from
Jan 29, 2023

Conversation

devanshj
Copy link
Contributor

@devanshj devanshj commented Jan 27, 2023

Supercedes #1565.

The recipe added in #1565 is unnecessarily wordy beyond measure. That's not to blame the author (or the maintainers) as they might not be aware of how the TypeScript guide is supposed to be. I just want to clarify my intention of the TypeScript guide as the original author: A succint to-the-point document that shows you how to deal with TypeScript, period. (This ignores the extra collapsed sections of course, which is precisely why they are collapsed.)

Which means it's not there to teach you how to use zustand patterns or anything. If you look at every recipe, it's literally just a snippet to show you the typings. So when the user comes to the "slices pattern" recipe for example, it is assumed they know what "slices pattern" already is, hence we don't teach slices pattern here but instead link at a detailed explanation at the end if they aren't familiar with it.

This is the mistake that #1565 does, it's way too tutorialy (has "steps" and is worded like so), has too broad scope (talks about testing and stuff), repetitive in that it shows how to use middlewares, mentions the extra parenthesis etc (those are not needed here).

So my suggested change is in this PR. If we want to "teach" this bounded useStore pattern (well it's not much of a pattern, it's just partial application) then it can be done in the readme or a seperate guide and then link it at the end as we do for slices pattern recipe.

Just my two cents, please feel free to close this PR if you don't like it.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 27, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2c46051:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
Next.js Configuration
@pavlobu/zustand demo Configuration

function useBearStore(): BearState
function useBearStore<T>(selector: (state: BearState) => T, equals?: (a: T, b: T) => boolean): T
function useBearStore<T>(selector?: (state: BearState) => T, equals?: (a: T, b: T) => boolean) {
return useStore(bearStore, selector!, equals)
Copy link
Contributor Author

@devanshj devanshj Jan 27, 2023

Choose a reason for hiding this comment

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

We need the ! after selector because our types are incorrect, they don't allow writing useStore({} as StoreApi<unknown>, undefined, () => true). I had told @dai-shi about this in #725 but he said that passing undefined is an incorrect usage. I personally think in most cases types should remain true to the implementation otherwise problems like this pop up.

I mean it's not a big deal but still in case if you change your mind and want to make our types more correct then let me know I'll open a PR (we just need to add a ? after selector in out useStore type).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I still think this is good. (But, I may change my mind in the future. In that case, I think I know what to do.)

@devanshj
Copy link
Contributor Author

run prettier

Ah yes prettier made my code uglier yet again :P

(as the react store is already a bounded useStore hook)
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

LGTM.

Apart from this PR, I still think not everybody has to read this guide fully. While sections may work, I wonder if we can split the guide into two parts: one for beginners, and the other for exports. The border would be difficult to define though. Or, maybe this guide is for latter, and we need something new for the former.

@dai-shi dai-shi requested review from sewera and chrisk-7777 January 29, 2023 07:52
@devanshj
Copy link
Contributor Author

I still think not everybody has to read this guide fully.

Oh that's absolutely true. The first two sections ie "Basic usage" and "Using middlewares" is sufficient to address most of the TypeScript usage.

So perhaps a split can be done there, a "TypeScript usage" page that has these two sections and then a "TypeScript advanced usage" that has remaining ones. Or something like that, pages could be named differently too, just giving an overall idea.

@dai-shi
Copy link
Member

dai-shi commented Jan 29, 2023

So perhaps a split can be done there

I personally like that. Let's see how others think.

@sewera
Copy link
Collaborator

sewera commented Jan 29, 2023

a split can be done there, a "TypeScript usage" page that has these two sections and then a "TypeScript advanced usage" that has remaining ones

I agree with this approach

@holgergp
Copy link
Contributor

Thanks for cleaning up the docs.
The idea behind it was, that I was missing those parts in the docs when using zustand. "Am I doing things right/like it is intended?" If I am wondering about that, chances are, that there are others wondering as well.
I chose the tutorial-ish style, as this is the style I like to read.

The idea of splitting the typescript docs into a basic and an advanced part sounds good to me!

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Let's merge this.

@dai-shi dai-shi merged commit f508b2c into pmndrs:main Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants