Local storage

[dewanshDT]

Fixes #2144

Changes: these changes allows the editor to store the theme locally for the next refresh

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[dewanshDT]

@lindapaiste @raclim I suggest we store all the preferences locally and I also want to organize the constants as mentioned in the todos of that file.

[lindapaiste]

This is a good idea but I think it needs more thought and discussion.

• My first question was how this interacts with the settings that are currently fetched from the API for logged-in users and which value takes priority. I looked at the code and it seems like the initialState would be set from the localStorage value and then it would get overwritten by the setPreferences action in the validateAndLoginUser thunk. So the API one has priority which is good.
• Interacting with localStorage is a side effect so as a Redux best practice it should not be in the reducer. As a Redux person I would put it in a middleware but this repo doesn't follow a lot of Redux best practices and doesn't use other middleware 🤷 So I'm not sure how to balance wanting to keep this a simple change vs not wanting to make the codebase any worse.
• If we are storing the theme preference, then why not store the language preference and others? Perhaps we can store the entire preferences state. We do that with the project state.

[dewanshDT]

Yess I also agree with this we should definitely store all the preferences locally

[dewanshDT]

What approach u think would be the best for the localstorage

[lindapaiste]

Tough question because like I said we are dealing with a lot of code which is already not great. We would JSON.stringify the whole preferences state and call localStorage.setItem(PREFERENCES_KEY, JSON.stringify(state.preferences)) but I'm not sure where to put it.

The API call to update the preferences for logged-in users is handled by thunk actions in the actions/preferences.js file so that's the most natural place with the least amount of change. In each action we have a block like this:

const state = getState();
    if (state.user.authenticated) {
      const formParams = {
        preferences: {
          fontSize: value
        }
      };
      updatePreferences(formParams, dispatch);
    }

Instead of a conditional execution with if, we could pass the state.user.authenticated boolean as an argument to the updatePreferences helper function. Then inside that function we would either call the API or update the localStorage, depending on whether the user is logged in or not.

The above code would become:

const state = getState();
      const formParams = {
        preferences: {
          fontSize: value
        }
      };
      updatePreferences(formParams, dispatch, state.user.authenticated);

and then the helper would become:

function updatePreferences(formParams, dispatch, isAuthenticated) {
  if (isAuthenticated) {
    // current API logic
  } else {
    // new localStorage logic.
  }
}

The formParams only contains the property which just changed so you would have to merge it with the current value by reading the localStorage. Or by looking at the rest of the preferences state?

---

Re: middleware. We can't easily check if an action is a preferences action because the actions don't follow the sort of consistent naming pattern that redux toolkit actions do so we can't use the startsWith that I recommended here. I'm thinking maybe the middleware applies the action and sees if the preferences state has changed? The other option is to check against the huge list of preferences action types but that's the sort of fragile code that I hate.

I'm busy today but I can play this more tomorrow. I'm also really tempted to put in a PR that rewrites this whole reducer and its actions. This is an example where the legacy code is hard to work with and makes it hard to add new features.

[dewanshDT]

I'll get back to this

[dewanshDT]

Hey @lindapaiste, sorry got stuck with some other things at that time. Before you write a PR that rewrites this whole reducer and Its actions would you like me to make the code changes you suggested in actions/preferences.js. Can I also help you with that PR of yours?

[lindapaiste]

@dewanshDT I actually did end up rewriting the whole reducer but I tried it two different ways and I haven't put in a PR because I'm not sure which one I prefer. I have a tendency to over-analyze things 🙃

I think that you should go ahead and handle the localStorage syncing within the thunk actions. It's a core rule of redux that reducers should not have side effects, but beyond that there are a lot of places you can put it, and thunks are one of the okay places (a thunk is an action creator which is a function of dispatch). We already have all of the preferences actions written as thunks due to the API call.

You can probably model it after the code that we have for sessionStorage here and here. I don't fully understand why we have that code but it looks like the purpose there was to keep the editor state when a user logs in.
#334

[lindapaiste]

@dewanshDT I actually did end up rewriting the whole reducer but I tried it two different ways and I haven't put in a PR because I'm not sure which one I prefer. I have a tendency to over-analyze things 🙃

Ok let me get this down in writing.

Approach 1 - Separate action creators for each action. This generally makes the most sense, but the downside here is that I am sending the entire preferences object to the API on each change so it's a larger payload being set across the network. Which I think I could fix but the code is really simple to just send the entire state.preferences.

reducer/slice file

import { createSlice } from '@reduxjs/toolkit';
import * as ActionTypes from '../../../constants';

// Note: structure matches the schema from the server.
const initialState = {
  fontSize: 18,
  autosave: true,
  linewrap: true,
  lineNumbers: true,
  lintWarning: false,
  textOutput: false,
  gridOutput: false,
  theme: 'light',
  autorefresh: false,
  language: 'en-US',
  autocloseBracketsQuotes: true
};

const preferences = createSlice({
  name: 'preferences',
  initialState,
  reducers: {
    setFontSize: (state, action) => {
      state.fontSize = action.payload;
    },
    setAutosave: (state, action) => {
      state.autosave = action.payload;
    },
    setLinewrap: (state, action) => {
      state.linewrap = action.payload;
    },
    setLineNumbers: (state, action) => {
      state.lineNumbers = action.payload;
    },
    setLintWarning: (state, action) => {
      state.lintWarning = action.payload;
    },
    setTextOutput: (state, action) => {
      state.textOutput = action.payload;
    },
    setGridOutput: (state, action) => {
      state.gridOutput = action.payload;
    },
    setTheme: (state, action) => {
      state.theme = action.payload;
    },
    setAutorefresh: (state, action) => {
      state.autorefresh = action.payload;
    },
    setLanguage: (state, action) => {
      state.language = action.payload;
    },
    setAutocloseBracketsQuotes: (state, action) => {
      state.autocloseBracketsQuotes = action.payload;
    }
  },
  // Replace the preferences when receiving a user object from the API.
  extraReducers: (builder) =>
    builder.addCase(
      ActionTypes.AUTH_USER,
      (state, action) => action.user.preferences
    )
});

export default preferences.reducer;

export const { actions } = preferences;

actions/middleware file

import * as ActionTypes from '../../../constants';
import listenerMiddleware from '../../../middleware';
import apiClient from '../../../utils/apiClient';
import { actions } from '../reducers/preferences';

export const {
  setFontSize,
  setAutosave,
  setLinewrap,
  setLineNumbers,
  setLintWarning,
  setTextOutput,
  setGridOutput,
  setTheme,
  setAutorefresh,
  setLanguage,
  setAutocloseBracketsQuotes
} = actions;

// Need to call the API whenever a logged-in user changes their preferences.
listenerMiddleware.startListening({
  // Match all actions from the preferences slice.
  matcher: (action) => action.type.startsWith('preferences/'),
  effect: (action, { dispatch, getState }) => {
    const state = getState();
    if (state.user.authenticated) {
      apiClient
        .put('/preferences', { preferences: state.preferences })
        .catch((error) => {
          const { response } = error;
          dispatch({
            type: ActionTypes.ERROR,
            error: response.data
          });
        });
    }
  }
});

Approach 2 - This has only one core action to update the state. Then I have action creator functions as wrappers around that action for each specific preference. That way I know when I make the API call which specific properties have changed and can just send those.

reducer/slice file

import { createSlice } from '@reduxjs/toolkit';
import * as ActionTypes from '../../../constants';

// Note: structure matches the schema from the server.
const initialState = {
  fontSize: 18,
  autosave: true,
  linewrap: true,
  lineNumbers: true,
  lintWarning: false,
  textOutput: false,
  gridOutput: false,
  theme: 'light',
  autorefresh: false,
  language: 'en-US',
  autocloseBracketsQuotes: true
};

const preferences = createSlice({
  name: 'preferences',
  initialState,
  reducers: {
    // Merge the partial preferences in the payload with the current options.
    setPreferences: (state, action) => Object.assign(state, action.payload)
  },
  // Replace the preferences when receiving a user object from the API.
  extraReducers: (builder) =>
    builder.addCase(
      ActionTypes.AUTH_USER,
      (state, action) => action.user.preferences
    )
});

export default preferences.reducer;

export const { setPreferences } = preferences.actions;

actions/middleware file

import listenerMiddleware from '../../../middleware';
import apiClient from '../../../utils/apiClient';
import * as ActionTypes from '../../../constants';
import { setPreferences } from '../reducers/preferences';

// Need to call the API whenever a logged-in user changes their preferences.
listenerMiddleware.startListening({
  actionCreator: setPreferences,
  effect: (action, { dispatch, getState }) => {
    const state = getState();
    if (state.user.authenticated) {
      apiClient
        .put('/preferences', { preferences: action.payload })
        .catch((error) => {
          const { response } = error;
          dispatch({
            type: ActionTypes.ERROR,
            error: response.data
          });
        });
    }
  }
});

// Specific action creators:

export function setFontSize(value) {
  return setPreferences({
    fontSize: value
  });
}

export function setLineNumbers(value) {
  return setPreferences({
    lineNumbers: value
  });
}

export function setAutocloseBracketsQuotes(value) {
  return setPreferences({
    autocloseBracketsQuotes: value
  });
}

export function setAutosave(value) {
  return setPreferences({
    autosave: value
  });
}

export function setLinewrap(value) {
  return setPreferences({
    linewrap: value
  });
}

export function setLintWarning(value) {
  return setPreferences({
    lintWarning: value
  });
}

export function setTextOutput(value) {
  return setPreferences({
    textOutput: value
  });
}

export function setGridOutput(value) {
  return setPreferences({
    gridOutput: value
  });
}

export function setTheme(value) {
  return setPreferences({
    theme: value
  });
}

export function setAutorefresh(value) {
  return setPreferences({
    autorefresh: value
  });
}

export function setAllAccessibleOutput(value) {
  return setPreferences({
    textOutput: value,
    gridOutput: value
  });
}

export function setLanguage(value) {
  return setPreferences({
    language: value
  });
}

Both approaches are using createSlice and createListenerMiddleware from Redux Toolkit. Plus a few small changes in other files (setting up the middleware, and changes to user actions where they interact with the preferences).

Where I left off with this is thinking that Approach 1 is definitely better but that I should see if I can figure out how to send a smaller API payload by looking at the changes in the state. Maybe it doesn't matter.

[lindapaiste]

Where I left off with this is thinking that Approach 1 is definitely better but that I should see if I can figure out how to send a smaller API payload by looking at the changes in the state.

...and it took longer to type that out than to fix it! Only a few extra lines to send a smaller payload with the help of lodash pickBy.

...
effect: (action, { dispatch, getState, getOriginalState }) => {
    const state = getState();
    const prevState = getOriginalState();
    if (state.user.authenticated) {
      const changes = pickBy(
        state.preferences,
        (value, key) => prevState.preferences[key] !== value
      );
      apiClient.put('/preferences', { preferences: changes })
...      

I haven't put in the localStorage logic, but I'm imagining that the setItem goes in an else branch of the if (state.user.authenticated) {

[dewanshDT]

@dewanshDT I actually did end up rewriting the whole reducer but I tried it two different ways and I haven't put in a PR because I'm not sure which one I prefer. I have a tendency to over-analyze things 🙃

Ok let me get this down in writing.

Approach 1 - Separate action creators for each action. This generally makes the most sense, but the downside here is that I am sending the entire preferences object to the API on each change so it's a larger payload being set across the network. Which I think I could fix but the code is really simple to just send the entire state.preferences.

reducer/slice file
actions/middleware file
Approach 2 - This has only one core action to update the state. Then I have action creator functions as wrappers around that action for each specific preference. That way I know when I make the API call which specific properties have changed and can just send those.

reducer/slice file
actions/middleware file
Both approaches are using createSlice and createListenerMiddleware from Redux Toolkit. Plus a few small changes in other files (setting up the middleware, and changes to user actions where they interact with the preferences).

Where I left off with this is thinking that Approach 1 is definitely better but that I should see if I can figure out how to send a smaller API payload by looking at the changes in the state. Maybe it doesn't matter.

Approach 1 defines separate action creators for each preference option and adds an extra reducer to the slice that handles updating the entire preferences object. The middleware listens for actions from the preferences slice and dispatches a PUT request to the API with the entire preferences object.

Approach 2 defines a single action creator, setPreferences, that takes an object with partial preference options as its payload. The middleware listens specifically for this action creator and dispatches a PUT request to the API with the payload.

Overall, Approach 2 is more concise and easier to maintain since it uses a single action creator for all preference options. Additionally, the middleware specifically listens for the setPreferences action creator, making it more targeted and efficient.

@lindapaiste let me know, what you think.

[raclim]

Thanks so much for all of your work on this! I don't think we're going to implement this for now, but might be something to potentially explore in the future!