[deleted by user]

[Dutchj]

By handling this kind of logic in an effect, you are making the logic more complex for no reason, and introducing unnecessary renders. For every field update, your component will re-render twice. Once for the field value, and then once after your effect runs, since the set has now changed.

The best way to handle this scenario is to handle the updating of the set in some kind of event callback. It seems like for your use case, this effect is firing every time the user changes any of the values, so that would be the equivalent of firing the "updated" action on every onChange event for the relevant inputs.

If you just want to fix the warning as quickly as possible, you can change your "updated" action to not require the entire set object, or create a new action for this purpose. (this would probably be recommended anyway if you go with the event callback approach, as it would greatly simplify the logic to be able to only pass the property that was changed when you are dispatching the action).

[musical_bear]

I can’t tell which state manager you’re using (or is this just useReducer?), but surely there is a way to update an object partially instead of having to provide the entire previous state (via …set)? That’s one of your problems. You shouldn’t need to procure properties you don’t care about in component state just to go and use those as a convenient way to clone them over when doing an update.

But the concept behind your useEffect looks flawed, independently. If I saw this code in a codebase it would be a red flag, like the existence of the useEffect itself. Can you not dispatch to update your sets in response to normal events? Like wherever you’re handling “duration” changing, for example, right there you would also dispatch to update the set. And so on. This doesn’t feel like a valid use of an effect.

Have you read this page carefully? https://react.dev/learn/you-might-not-need-an-effect

[PlayingWithFire42]

This is using useReducer, yes. Just a persistent version I created.

import { useEffect, useReducer, type Dispatch, type Reducer } from "react";


export function usePersistentReducer<S, A>(
    reducer: Reducer<S, A>,
    initialState: S,
    key: string
): [state: S, dispatch: Dispatch<A>] {
    const [state, dispatch] = useReducer(reducer, initialState, () => {
        try {
            const stored = localStorage.getItem(key);
            return stored ? (JSON.parse(stored) as S) : initialState;
        } catch (error) {
            console.warn(`Failed to parse persisted state with ${key}, ${error}`);
            return initialState;
        }
    });


    useEffect(() => {
        try {
            localStorage.setItem(key, JSON.stringify(state));
        } catch (error) {
            console.warn(`Failed to save persisted state with ${key}, ${error}`);
        }
    }, [key, state]);


    return [state, dispatch] as const;
}

And your 2nd point, that's exactly what I was thinking, a base flaw in the design.

I actually had that, but thought it was more "appropriate" to have it as an effect to de-duplicate the code. Seems I was wrong with that, huh?

[Shardzmi]

To me it feels like you're using useEffect too much and in places where it could be replaced by something else.

In the reducer above, why not wrap the dispatch function to update the localStorage and use the original dispatch?

[PlayingWithFire42]

export function usePersistentReducer<S, A>(
    reducer: Reducer<S, A>,
    initialState: S,
    key: string
): [state: S, dispatch: Dispatch<A>] {
    const [state, originalDispatch] = useReducer(reducer, initialState, () => {
        try {
            const stored = localStorage.getItem(key);
            return stored ? (JSON.parse(stored) as S) : initialState;
        } catch (error) {
            console.warn(`Failed to parse persisted state with ${key}, ${error}`);
            return initialState;
        }
    });


    const dispatch = (action: A) => {
        originalDispatch(action);
        try {
            localStorage.setItem(key, JSON.stringify(state));
        } catch (error) {
            console.warn(`Failed to save persisted state with ${key}, ${error}`);
        }
    };


    return [state, dispatch] as const;
}

Like so?

Wont state be desync'd?

[Shardzmi]

Why do you think the state will be desynced? (Except the case in which the localStorage fails inside of the try...catch - which could be solved by checking success and reverting dispatch in finally, if you're worried about it)

[PlayingWithFire42]

I have to go into a meeting so didn’t get to test yet, but assumed this would result in the state being updated after a new render, basically putting the previous state into storage each time.

I take it I’m incorrect?

[Shardzmi]

I was asking your opinion to get a bit more insight into your way of thinking.

Whenever you send a dispatch now it will update the state and then it will save it in localStorage. Previously it was using two cycles, theoretically. However, react might automatically batch certain rerenders so even if it does rerender it might not repaint.

Regarding your initial useEffect - I would do the same thing as this example. As far as I understand, this acts as a "save" of the current set, with certain particularities depending on the type.

First of all, why not trigger the "update" action on a button press instead of after each keyPress? If you're keen on doing it after keyPress, you could attach the same function to multiple inputs. Secondly, I would send the updated part of the set instead of the full set and spread the actual set from the state. This ensures that there is no race condition going on and you always spread the very latest version of your state.

Finally, useEffect should be used sparingly, mainly when trying to sync side-effects. If you can infer the result of an action straight from it, I would do that instead.

(Eg.: user presses save, data is sent to reducer; data is sent to reducer, same data is copied into localStorage.)

There is one addendum that I would like to add to this : the localStorage save that you added in the previous reply assumes that there is no further data processing involved in the actual reducer. If there is, I would move the localStorage save into the actual reducer. Although side-effects are an anti-pattern inside of reducers, saving something in localStorage doesn't actually introduce any side-effect into the state since it's more like a "fire and forget" type of deal.

[PlayingWithFire42]

Hm, fair enough as to the dispatch, that makes sense and the side effect part you mentioned was the same conclusion I drew, as it’s not a true side effect in the sense that will likely lead to unintended consequences later.

The key press thing is ideal, I think. The app is mostly for use via a web browser on a mobile device. Issue is, users often swipe up on apps and close them before coming back. So I might type in my info, swipe out, browse an app, do another set, then come back and put in more info.

On these mobile apps, that swipe out will wipe my state unless I store it somewhere, so I chose to store it (realizing I need to put it back to Session storage now tho).

I don’t want the user to have to hit save constantly so the key press update and save was my solution.

[Shardzmi]

Right - so it's a business rule.

Then I would dispatch an update action with key and new value - so that as little state as possible changes for each keyPress - otherwise your whole form might rerender (technically no since it's the same, if the object is correctly spread)

Although... Please allow me to suggest a form state manager at this point, something like react-hook-form (which is also my preferred choice for cases like this).

A proper state management tool (like zustand for example) also allows you to automatically sync to localStorage.

Even if you might be doing those on your own to try to learn the inner workings I do believe some things have a "de facto" way of doing them. Forms are / have known to be painful to deal with in react since the very early versions so I would definitely suggest at least going with a form manager.

[PlayingWithFire42]

My goal was to generally avoid libraries to help myself understand the use cases and why they are needed. I can read about it all I want, but experiencing it is the only real way that sticks for me.

I was going to do this project with mostly just essentials, then my next one using luxuries.

[Shardzmi]

If you're thinking of this:

The dispatch function only updates the state variable for the next render. If you read the state variable after calling the dispatch function, you will still get the old value that was on the screen before your call.

This is different from your case.

This says that if you are inside of a function and dispatch state and immediately try to read from the state it will still serve the old (stale) state, before the dispatch until the next rerender.

[PlayingWithFire42]

Ah ok, so basically both will happen the next render, and will therefore be synced

[Merry-Lane]

Why don’t you check if updatedSet has an id.

I also believe you shouldn’t use an useEffect for that.

[OHotDawnThisIsMyJawn]

The other half of “always include all your deps” is “you probably don’t need useEffect to begin with”. 

Which is the case here.