Allow first argument in `adapt()` to accept a factory function that produces an initial state

[Evertt]

This is coming from a discussion I had opened where you mentioned that you were planning to add sinks to adapt() which could then be used for side-effects such as persisting the state in localStorage.

But of course a feature like persisting state in localStorage wouldn't be complete without a way to also initialize the state from localStorage. And I believe that a factory function would provide that feature very nicely.

I'm opening this issue for this proposal, because if you approve it then I'll happily open a PR to implement it.

So just to make it super clear, it would allow one to replace adapt("Bob") with adapt(() => "Bob"), which could then obviously be used to do stuff like adapt(() => localStorage.getItem("name") ?? "Bob").

Let me know if you approve.

[mfp22]

Yeah this makes sense and is common in state management. Let me first fix a type inference issue with sources, then you can make sure all the tests still pass when you implement this. Type inference can be sensitive. I think the new NoInfer could help (after we upgrade to the latest TypeScript)

[mfp22]

I tabled the other issue, so if you want to work on this, feel free.

Notes: initialState probably should be initialized as getInitialState and replaced with () => initialState if it's not already a function, and initialState should be set to the result of immediately calling it if it's passed in as a function. This would be used for the part of the code with createUpdateReaction(); But after that, getInitialState will be called inside the defer with the comment // Runs first upon subscription and in getStateSelector.

The complication is the React integration. As soon as a store is created, I put initialState in it because useEffect doesn't run immediately to subscribe to the store, so we need initial results synchronously based off of initialState. This store property should probably be changed to getInitialState.

However, to simplify the integration and make it more generic, I changed the state selector (defined in the function getStateSelector) for all stores to check if the store has been initialized, and just use the initialState defined with the store if it hasn't been. I think this might be useful for signal integrations too. Anyway, getStateSelector could just be changed to use getInitialState as well. getStateSelector won't call it unless the store is inactive, so it will never get in the way of the currently active state.

As the store gets subscribed to, it will define the initialState inside the defer mentioned above. So when the store gets .reset() called on it, it will return to the actual initial state when the store got activated the latest time rather than calling the function again and getting a totally new value.

The documentation will need to be updated for RxJS, Angular and React.

Is there a better name for getInitialState?

Let me know if you want to work on this. There's a chance I missed a detail, but I don't think so.

[Evertt]

@mfp22 Sorry I've been silent recently. The amount of time and energy I have available tends to fluctuate a lot. But I'm hoping to start working on this today.

edit

Sigh, okay it might take a while longer still, sorry. 😓

[Evertt]

Okay I've started working on this and unfortunately I do not fully understand everything you've said.

So far I have simply changed the initialState argument in adapt() into newInitialState: MaybeFactory<State> and then already in the first few lines of the function body I get the initial state like so:

// Initialize parameters
const getInitialState = typeof newInitialState === 'function'
  ? newInitialState as Factory<State> : () => newInitialState;

const initialState = getInitialState();

And I have indeed changed getStateSelector() to use getInitialState instead of initialState. But I don't really understand what you meant with the rest of what you said. You mentioned initialState getting defined inside a defer, which I guess could refer to this line or this line. But I'm not sure if, and what, you'd want me to change anything about those lines.

I also found this line that seems related to what you mentioned related to React's requirements. Also here I wonder whether you meant for me to do anything about this.

Shall I just open up a draft pull request to show you the changes so far? They're quite minimal, but I guess it'll be easier for talking about it.

[mfp22]

I would probably spend more time helping you get this right than just implementing it myself. I appreciate the contribution, but I explained everything you would need to know in the last comment if you understood enough about StateAdapt to make a change to the core functionality.

For now, in general, what would help most is writing clear, concise but detailed issues so it's clear to me what your exact problem is so I can think about how the API could change to support it. Then I can know what needs to be done and I can just get it done quickly.

One extra thing that would help a lot is if you wanted to give me some code for additional test cases I can test against. This would be a good place for it libs/rxjs/src/lib/global-store/state-adapt.spec.ts You could test that the factory function works, and not just the first time the store is created, but the 2nd time returning the correct thing, and when you call store.reset() the 2nd time it resets to the 2nd initialState returned by the factory function at the time the 2nd store was created. If this doesn't make sense then I can just do it anyway.

[Evertt]

I'm sorry I couldn't be more helpful. I hope that maybe in the future I'll understand your codebase better and be able to make changes better too.

For now I will indeed just try to make clear, concise and detailed issues whenever I personally think things can be improved.

And yes, I will try to add some additional test cases for this current update where initialData could be either a simple State or a Factory<State>. To test whether the store that comes out of adapt() still behaves well whenever a state factory is used and also check whether none of the old tests fail with my changes.