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

refactor(react): optimize react layer #5000

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

TkDodo
Copy link
Contributor

@TkDodo TkDodo commented Jul 27, 2024

  • replace use-sync-external-store/shim with useSyncExternalStore from react
  • do not memoize getSnapshot in uSES
  • implement getServerSnapshot in uSES
  • expect store to always be defined
  • update react types to v18 and testing library to v16

- replace use-sync-external-store/shim with useSyncExternalStore from react
- do not memoize getSnapshot in uSES
- implement getServerSnapshot in uSES
- expect store to always be defined
- update react types to v18 and testing library to v16
Copy link

changeset-bot bot commented Jul 27, 2024

🦋 Changeset detected

Latest commit: 2e9fa22

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@xstate/store Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TkDodo
Copy link
Contributor Author

TkDodo commented Jul 27, 2024

it looks like some typings in tests are failing after I updated the types to react18 (needed to be able to import useSyncExternalStore from react directly), but only in xstate-react. It's always:

error TS2322: Type 'StateValue' is not assignable to type 'ReactNode'.

when trying to render state.value. I'm not sure how to fix this - isn't this something users would also see when using react18 typings?

@Andarist
Copy link
Member

Do u have any numbers for this optimization? Where the costs are in the old solution? This would be a breaking change so it will require a new major if we decide to land this

@TkDodo
Copy link
Contributor Author

TkDodo commented Jul 27, 2024

Do u have any numbers for this optimization? Where the costs are in the old solution? This would be a breaking change so it will require a new major if we decide to land this

why is it a breaking change? because store can no longer be undefined ? I can revert that change if you think it's breaking, but I think it's undocumented and passing undefined to useSelector serves no purpose at the moment, so I think removing it is fine.

To see the concrete differences in bundle size, I would need to gather a preview build of some sorts. But the uSES shim alone has about 3.11kb, and useSelector from @xstate/store/react seems to be 3.23kb in total, so I would say it's a significant decrease given that the shim should be unnecessary as you're not supporting React v17. Granted, this doesn't account for the new code I've written (useSelectorWithCompare) so the diff won't be that large, but still significant.

@davidkpiano
Copy link
Member

Do u have any numbers for this optimization? Where the costs are in the old solution? This would be a breaking change so it will require a new major if we decide to land this

I'm fine with releasing this as a new major

@davidkpiano
Copy link
Member

it looks like some typings in tests are failing after I updated the types to react18 (needed to be able to import useSyncExternalStore from react directly), but only in xstate-react. It's always:

error TS2322: Type 'StateValue' is not assignable to type 'ReactNode'.

when trying to render state.value. I'm not sure how to fix this - isn't this something users would also see when using react18 typings?

In general, the StateValue can either be a "string" or, to represent nested/parallel state values, { an: 'object' }. React is right to complain here, since <div>{{ foo: 'bar' }}</div> won't render. In the near future though, we'll be able to narrow the state.value type so that we can actually know if it will only be a string, which is the case in these examples.

I made a patch that fixes the type issues (click to see)
diff --git a/packages/xstate-react/test/createActorContext.test.tsx b/packages/xstate-react/test/createActorContext.test.tsx
index 27356bdc21..cf12ae8eb4 100644
--- a/packages/xstate-react/test/createActorContext.test.tsx
+++ b/packages/xstate-react/test/createActorContext.test.tsx
@@ -37,7 +37,7 @@ describe('createActorContext', () => {
     const SomeContext = createActorContext(someMachine);
 
     const Component = () => {
-      const value = SomeContext.useSelector((state) => state.value);
+      const value = SomeContext.useSelector((state) => state.value as string);
 
       return <div data-testid="value">{value}</div>;
     };
@@ -76,7 +76,7 @@ describe('createActorContext', () => {
 
       return (
         <>
-          <div data-testid="value">{state.value}</div>
+          <div data-testid="value">{state.value as string}</div>
           <button
             data-testid="next"
             onClick={() => actorRef.send({ type: 'NEXT' })}
@@ -196,7 +196,7 @@ describe('createActorContext', () => {
       const actor = SomeContext.useActorRef();
       const value = useSelector(actor, (state) => state.value);
 
-      return <div data-testid="value">{value}</div>;
+      return <div data-testid="value">{value as string}</div>;
     };
 
     const App = () => {
@@ -412,7 +412,7 @@ describe('createActorContext', () => {
             actorRef.send({ type: 'next' });
           }}
         >
-          {state.value}
+          {state.value as string}
         </div>
       );
     };
diff --git a/packages/xstate-react/test/useActor.test.tsx b/packages/xstate-react/test/useActor.test.tsx
index eed3f6a0dd..ea8b20c30b 100644
--- a/packages/xstate-react/test/useActor.test.tsx
+++ b/packages/xstate-react/test/useActor.test.tsx
@@ -694,7 +694,7 @@ describeEachReactMode('useActor (%s)', ({ suiteKey, render }) => {
       const [state, send] = useActor(machine);
       return (
         <>
-          <div data-testid="result">{state.value}</div>
+          <div data-testid="result">{state.value as string}</div>
           <button onClick={() => send({ type: 'EV' })} />
         </>
       );
@@ -740,7 +740,7 @@ describeEachReactMode('useActor (%s)', ({ suiteKey, render }) => {
       );
       return (
         <>
-          <div data-testid="result">{state.value}</div>
+          <div data-testid="result">{state.value as string}</div>
           <button onClick={() => send({ type: 'EV' })} />
         </>
       );
diff --git a/packages/xstate-react/test/useActorRef.test.tsx b/packages/xstate-react/test/useActorRef.test.tsx
index 6739ec7230..a802e587b9 100644
--- a/packages/xstate-react/test/useActorRef.test.tsx
+++ b/packages/xstate-react/test/useActorRef.test.tsx
@@ -193,7 +193,7 @@ describeEachReactMode('useActorRef (%s)', ({ suiteKey, render }) => {
           >
             Send to child
           </button>
-          <div data-testid="child-state">{childState.value}</div>
+          <div data-testid="child-state">{childState.value as string}</div>
         </>
       );
     };
@@ -251,7 +251,7 @@ describeEachReactMode('useActorRef (%s)', ({ suiteKey, render }) => {
           >
             Send to child
           </button>
-          <div data-testid="child-state">{childState.value}</div>
+          <div data-testid="child-state">{childState.value as string}</div>
         </>
       );
     };
@@ -325,7 +325,7 @@ describeEachReactMode('useActorRef (%s)', ({ suiteKey, render }) => {
 
   it('should work with a promise actor', async () => {
     const promiseLogic = fromPromise(
-      () => new Promise((resolve) => setTimeout(() => resolve(42), 10))
+      () => new Promise<number>((resolve) => setTimeout(() => resolve(42), 10))
     );
 
     const App = () => {
@@ -506,7 +506,7 @@ describeEachReactMode('useActorRef (%s)', ({ suiteKey, render }) => {
           >
             Send event
           </button>
-          <span>{value}</span>
+          <span>{value as string}</span>
         </>
       );
     }
@@ -565,7 +565,7 @@ describeEachReactMode('useActorRef (%s)', ({ suiteKey, render }) => {
           >
             Send event
           </button>
-          <span>{value}</span>
+          <span>{value as string}</span>
         </>
       );
     }
diff --git a/packages/xstate-react/test/useSelector.test.tsx b/packages/xstate-react/test/useSelector.test.tsx
index fc2c6552c3..3de48d3a97 100644
--- a/packages/xstate-react/test/useSelector.test.tsx
+++ b/packages/xstate-react/test/useSelector.test.tsx
@@ -682,7 +682,7 @@ describeEachReactMode('useSelector (%s)', ({ suiteKey, render }) => {
 
       return (
         <>
-          <div data-testid="child-state">{childState.value}</div>
+          <div data-testid="child-state">{childState.value as string}</div>
           <button
             data-testid="child-send"
             onClick={() => childRef.send({ type: 'NEXT' })}

@TkDodo
Copy link
Contributor Author

TkDodo commented Jul 28, 2024

Thanks @davidkpiano, I've applied your patch

# Conflicts:
#	packages/xstate-react/package.json
#	packages/xstate-store/package.json
#	yarn.lock
Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

cc. @Andarist for quick review

@davidkpiano davidkpiano merged commit eeadb71 into statelyai:main Jul 30, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Jul 30, 2024
@TkDodo TkDodo deleted the feature/refactor-react branch July 30, 2024 18:33
@TkDodo
Copy link
Contributor Author

TkDodo commented Jul 30, 2024

🎉

will you ship this as a major or minor?

@davidkpiano
Copy link
Member

🎉

will you ship this as a major or minor?

Likely a major since we changed how useSelector works

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.

3 participants