Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Upgrade hookstate #10170

Merged
merged 13 commits into from
May 17, 2024
Merged

Upgrade hookstate #10170

merged 13 commits into from
May 17, 2024

Conversation

speigg
Copy link
Member

@speigg speigg commented May 16, 2024

Summary

I made the following changes in a fork of hookstate that this branch is pulling in. This should be carefully reviewed.

  • HOOKSTATE-111: Previously, this error was thrown more than any other hookstate error in our codebase (due to the complexity around our ECS hooks), and has been responsible for a lot of our crashing.
    • Now: this error no longer occurs, as it's now valid to dynamically change the store referenced by a useHookstate(); This propagates all the way to our ECS hooks. It's now valid to use patterns where the entities are dynamically changed in the context of a single reactor:
const myComponent = useComponent(someEntity, MyComponent) // someEntity can change between renders!

Previously, this either crashed, or (if the behavior was needed) required a very awkward work-around w/ sub-reactors. IMO, just this one change is a HUGE improvement.

Other very minor (one-line) changes:

  • React throws errors when setting values before a component is done mounting - This was due to hookstate calling setValue in reactors that haven't finished mounting yet, due to state changes in hooked global states.

    • Now: hookstate will only call setValue after the component has mounted. This error should no longer be caused by hookstate. This change is defininitely worth keeping IMO.
  • HOOKSTATE-106: This error is caused by attempting to update a state after it has already been destroyed (e.g., due to unmounting). I've seen this error a few times in our engine. In React, this scenario is essentially ignored (React doesn't seem to care at all of if setValue is called after it's context has unmounted and state destroyed).

    • Now: hookstate will still attempt to update the hooked global state if it has a reference to it, and will print a warning instead of throwing an error in the console. This is a small thing, could take it or leave it (and probably, the root cause of the problem here would be asynchronous tasks in side-effects that haven't been properly cleaned up). Hookstate source code already had a TODO to turn this error into a console warning, so I did that.

Subtasks Checklist

Still a draft, needs some cleanup.

Breaking Changes

  • The latest version of hookstate types state.value and state.get() as read-only (this is nice for type-safety)

    • we might want to borrow their Immutable<> type for use on getState() and getComponent() return values.
  • This old pattern: useHookstate(getMutableState(X)) now produces broken typings that I can't fix.

    • The suggested api is: useMutableState(X) (this one has working types)
  • The hookstate plugin API changed, and is now an extension api.

    • Our syncStateWithLocalStorage plugin has been rewritten to the new extension format
    • defineState(...) API now accepts an extension property for adding hookstate extensions

References

closes #insert number here

QA Steps

@AidanCaruso AidanCaruso self-requested a review May 16, 2024 20:15
@HexaField HexaField marked this pull request as ready for review May 17, 2024 02:51
@HexaField HexaField self-requested a review May 17, 2024 02:51
@HexaField HexaField merged commit 55c934e into dev May 17, 2024
13 checks passed
@HexaField HexaField deleted the upgrade-hookstate branch May 17, 2024 03:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants