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

feat(vanilla): non-object state #1144

Merged
merged 12 commits into from Aug 18, 2022
Merged

feat(vanilla): non-object state #1144

merged 12 commits into from Aug 18, 2022

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Jul 27, 2022

and simplify types (eliminating Cast).


There might not be many use cases, but maybe we can support non-object state.

Counter example: https://codesandbox.io/s/react-typescript-forked-uvyys3?file=/src/App.tsx

import create from "zustand";

const useCount = create(() => 0);
const inc = () => useCount.setState((c) => c + 1);

const Counter = () => {
  const count = useCount();
  return (
    <div>
      {count} <button onClick={inc}>+1</button>
    </div>
  );
};

TODOs

  • vanilla types&impl
  • react types
  • middleware
    • combine: object only
    • devtools: theoretically accepts non-object
    • immer: theoretically accepts non-object
    • persist: supports non-object
    • redux: theoretically accepts non-object
    • subscribeWithSelector: supports non-object
  • tests/basic.test.tsx

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 27, 2022

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 bfde4c2:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
React Typescript (forked) PR

@github-actions
Copy link

github-actions bot commented Jul 27, 2022

Size Change: +430 B (+1%)

Total Size: 29.4 kB

Filename Size Change
dist/esm/middleware.js 2.8 kB +71 B (+3%)
dist/esm/vanilla.js 402 B +17 B (+4%)
dist/middleware.js 3.3 kB +64 B (+2%)
dist/system/index.production.js 394 B +1 B (0%)
dist/system/middleware.development.js 2.9 kB +72 B (+3%)
dist/system/middleware.production.js 1.78 kB +32 B (+2%)
dist/system/vanilla.development.js 471 B +20 B (+4%)
dist/system/vanilla.production.js 285 B +15 B (+6%) 🔍
dist/umd/middleware.development.js 3.41 kB +58 B (+2%)
dist/umd/middleware.production.js 2.09 kB +32 B (+2%)
dist/umd/vanilla.development.js 607 B +17 B (+3%)
dist/umd/vanilla.production.js 390 B +12 B (+3%)
dist/vanilla.js 475 B +19 B (+4%)
ℹ️ View Unchanged
Filename Size
dist/context.js 518 B
dist/esm/context.js 464 B
dist/esm/index.js 426 B
dist/esm/middleware/immer.js 210 B
dist/esm/shallow.js 272 B
dist/index.js 670 B
dist/middleware/immer.js 373 B
dist/shallow.js 318 B
dist/system/context.development.js 589 B
dist/system/context.production.js 393 B
dist/system/index.development.js 602 B
dist/system/middleware/immer.development.js 292 B
dist/system/middleware/immer.production.js 187 B
dist/system/shallow.development.js 338 B
dist/system/shallow.production.js 253 B
dist/umd/context.development.js 662 B
dist/umd/context.production.js 488 B
dist/umd/index.development.js 816 B
dist/umd/index.production.js 570 B
dist/umd/middleware/immer.development.js 514 B
dist/umd/middleware/immer.production.js 371 B
dist/umd/shallow.development.js 447 B
dist/umd/shallow.production.js 344 B

compressed-size-action

src/vanilla.ts Outdated Show resolved Hide resolved
@dai-shi
Copy link
Member Author

dai-shi commented Jul 27, 2022

@devanshj Can you help typing middlewares? This is not in hurry.

@devanshj
Copy link
Contributor

devanshj commented Jul 27, 2022

@devanshj Can you help typing middlewares? This is not in hurry.

I think things should not change much, this would mainly involve removing extends State from places. Give it a try and then if you're finding it difficult then let me know and I'll help.

@dai-shi
Copy link
Member Author

dai-shi commented Jul 27, 2022

Yeah, I already tried and lost confident, especially around Cast.
I can try that again and ask for review if that's easier.

@devanshj
Copy link
Contributor

Yeah, I already tried and lost confident, especially around Cast.

Ah okay then I'll look into it hopefully soon.

I can try that again and ask for review if that's easier.

Yeah if you can tell me specific places where you got stuck then I can solve it and explain it to you and it will also make you understand the types better.

src/middleware/persist.ts Outdated Show resolved Hide resolved
@dai-shi dai-shi marked this pull request as ready for review July 27, 2022 15:22
@dai-shi dai-shi changed the title feat: non-object state feat(vanilla): non-object state Aug 18, 2022
@dai-shi dai-shi merged commit c60a535 into main Aug 18, 2022
@dai-shi dai-shi deleted the feat/non-object-state branch August 18, 2022 22:47
A extends Action,
Cms extends [StoreMutatorIdentifier, unknown][] = []
>(
type Redux = <T, A, Cms extends [StoreMutatorIdentifier, unknown][] = []>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@dai-shi Is removing the constraint in A intentional? This would allow non-redux actions (and non-redux reducers), which might not work well with devtools. I don't think it's a big deal to remove the constraint, but I'd prefer to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is intentional. (to remove Cast, which is more important for me.)
For 99% use cases, there should be no issues. btw, we have a type guard and work around to force { type: unknown }.

Copy link
Contributor

@devanshj devanshj Aug 18, 2022

Choose a reason for hiding this comment

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

to remove Cast, which is more important for me.

Instead of Cast<A, Action> we can write A & Action too1. We just have to satisfy the type-checker. Both Cast<A, Action> and A & Action are subtype of Action (which is what the type-checker wants), and when A is subtype of Action (which it'll always be) both mathematically resolve to just A. And we'll have to write A & Action at only one place.

btw, we have a type guard and work around to force { type: unknown }.

That makes it worse, right now we're sending isObjectWithTypeProperty(x) ? x : { type: x } that means if someone has an action { kind: "INCREMENT", by: 10 } (which is now allowed) what will get sent is { type: { kind: "INCREMENT", by: 10 } }.

For 99% use cases, there should be no issues.

That's true. It's not a huge problem, it's just a matter of correctness. And again, I'm fine eitherway.

Footnotes

  1. In case one is wondering why we used Cast and not & in the first place... Cast is prefered because even though Cast<A, Action> and A & Action both mathematically resolve to just A, the A & Action will still show & Action part in the quickinfo tooltip (which is useless because A is already a subtype of Action) whereas Cast<A, Action> literally (not just mathematically) resolves to A because A is returned as it is. So the answer is "to make the quickinfo nicer".

Copy link
Member Author

Choose a reason for hiding this comment

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

if someone has an action { kind: "INCREMENT", by: 10 } (which is now allowed)

I implicitly and intentionally make it loose. Redux Devtools allows { type: { kind: "INCREMENT", by: 10 } }, right?
(So, I really wanted to make State=unknown and Action=unknown.)

Copy link
Contributor

@devanshj devanshj Aug 19, 2022

Choose a reason for hiding this comment

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

Redux Devtools allows { type: { kind: "INCREMENT", by: 10 } }, right?

Yes but the action is { kind: "INCREMENT", by: 10 } and not { type: { kind: "INCREMENT", by: 10 } }. So when the user jumps or dispatches an action from the redux devtools, what we'll receive from the extenstion is { type: { kind: "INCREMENT", by: 10 } } which we'll dispatch and forward to the reducer but the reducer will simply won't recognise it.

The point I'm trying to make is all tooling, libraries, code around redux is written keeping in mind that the action will have a type property, so we can't allow violating the redux spec.

I personally see no point in violating the spec just for removing & Action from our code. But I'm fine if you're making that choice consciously. In that case maybe let's send typeof x === "object" ? x : { type: x } instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

which we'll dispatch and forward to the reducer but the reducer will simply won't recognise it.

Ah, I overlooked that.

Hm, I thought "our" redux middleware doesn't need to follow the type property spec, but this your comment makes me change the idea. Let me see how & Action works.

Copy link
Member Author

Choose a reason for hiding this comment

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

@devanshj
Copy link
Contributor

Let's also change the State type and the deprecation documentation...

 /**
- * @deprecated Use `object` instead of `State`
+ * @deprecated Use `unknown` instead of `State`
  */
- export type State = object
+ export type State = unknown

This should be probably be non-breaking as we have changed object to unknown everywhere. If you want to keep State as object then it's worth updating the deprecation documentation at least.

dai-shi pushed a commit to devanshj/zustand that referenced this pull request Aug 18, 2022
dai-shi pushed a commit that referenced this pull request Aug 18, 2022
SD11892 pushed a commit to SD11892/zustand that referenced this pull request Mar 15, 2023
crown0128 added a commit to crown0128/zustand that referenced this pull request Sep 21, 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.

None yet

2 participants