Break up `app.js`

[didoesdigital]

The app.js file is huge, disorganised, unformatted, has 1 giant app state, is a class component, and lacks types. I want to make app.js and changing Typey Type more manageable.

Background

Typey Type started out as a single component, single page (no routes) app about 7 years ago before React hooks existed. I've been progressively breaking down components into smaller, more manageable parts, with more folder structure, and updating them to TypeScript with a preference for function components so I can use hooks (see examples: https://github.com/didoesdigital/typey-type/pull/144, https://github.com/didoesdigital/typey-type/pull/145, https://github.com/didoesdigital/typey-type/pull/146, https://github.com/didoesdigital/typey-type/pull/147, https://github.com/didoesdigital/typey-type/pull/148, https://github.com/didoesdigital/typey-type/pull/149, https://github.com/didoesdigital/typey-type/pull/150). But app.js is so large, tangled, and unwieldy that it is difficult to break up.

Goals

• Having 1 giant app state makes it difficult to make changes and every change to state causes the entire app to re-render so having better state management would make it possible to reduce unnecessary and sometimes expensive re-renders, reduce prop drilling, and make it easier to make changes. I expect that would look like using a React Context provider for each user setting and for other small groups of app state.
• If App and everything in app.js was using function components, it would be easier to use hooks in more places, which are extremely helpful.
• If app.js were smaller, it would be easier to add types and convert components to TypeScript to make changes easier and more maintainable.
• Smaller, single-purpose files would be easier to read, understand, and update.
• Organised folders and files would make it easier to find relevant code.
• Breaking down a component to its own file in 1 commit and formatting the component in a separate commit would reduce noise in commits and make code consistently formatted and readable.

Approach

While I prefer structured refactoring with small, isolated, always-working commits, I may be reaching the limits of what's left in app.js that can be broken down a bit at a time.

[didoesdigital]

I'm looking for ideas on how to move this work forward.

So far, my current best plan is an ambitious re-write of the whole app:

• Start with a bit more prep work like consolidating all the functions to update lesson user settings, re-writing and improving the set/load personal preferences and local storage code, improve file loading e.g. dictionaries and lessons
• Make a new App.tsx with routes just for lessons, build just enough to make the critical parts of lessons work
• Then expand the lesson functionality…
• Then expand working routes and features…
• Deploy to an alternative, password-protected place for vigorous testing

This approach would halt all other progress on the app, take a long time and commitment, and have lots of subtle changes in behaviour to the app. I would love a better plan than this!

[na2hiro]

I also felt that App has a lot to manage, while working on the postfix overrun issue. I think this kind of giant state is typical in matured react apps: states are lifted to the root for sharing and root has lots of states.

State management library

Providing states as React Context you mentioned is one immediate solution to prop drilling, but I don't recommend it as a long-term solution because performance is not out of box and there are some caveats on implicit dependencies between context (e.g. if inner context value was read by outer context, outer context gets a default value of inner context without errors).

State management libraries come into this area and do a really good job. Redux has been a pioneer in this area, but I find Jotai useful with its simple and powerful concepts of atoms for defining states and derived data separated from component trees. (Recoil explains it very well with the videos their or the motivation page. It's very similar library to jotai from Meta but dev team has been laid off) I think Typey Type is a data-intensive app and it's a good match with the easy defining of derived data.

With the library, states and logic around them could be placed in separate directories and independent from component tree, and consumer of states or trigger of state updates would directly consume using hooks like useAtom(somethingState). As migration of states from App to atoms, less states and updating logic would be in App.js and eventually it could be empty.

Modified excerpts from https://github.com/na2hiro/typey-type/commit/227dbe9c88778f2427eecd0a8f85900b3fe27963#diff-512813af7a666922f452efd7095a944b07bf9d44976a2431a04737e934f9ec2c:

// (metWordsState.ts)
export const metWordsState = atom({} as MetWords);
// derived data. auto-updated when metWordsState changes
export const yourSeenWordCountState = atom((get) => calculateSeenWordCount(get(metWordsState)));

Example of consuming atoms

// (Consumer.tsx)
  const [metWords, setMetWords] = useAtom(metWordsState)

  const [yourSeenWordCount, _] = useAtom(yourSeenWordCountState)

Integrating state management library doesn't mean we need to switch everything at once. We can migrate state by state while keeping everything working. Since states can use another states for its updating logic etc, it's straightforward if underlying data is already managed by the same library. So the first candidate to migrate would be a state that doesn't depend on any other states. (userSettings might be one)

I made some PoC commits on this branch https://github.com/na2hiro/typey-type/commits/jotai and left comments for the important part of the changes. I think last two commits around metWords shows nice step-by-step refactoring that's applicable to other states. Feel free to leave inline comments if you have any questions there

Observation on App

Whether we go with a library or not, it's useful to observe what's in App and think about ways for incremental update towards more maintainable code. Here's my opinion

• Giant state (and the file)
  • I observe most of the methods are to manage state during a lesson (rough classification: https://github.com/na2hiro/typey-type/commit/b11aed790c4ffe6fe572c51778d8c3f45c840620). I think extracting non-lesson states from App would be a good step.
    • For example, userSettings could be managed outside of App as it will be read only by lessons
    • Once everything in App is lesson states, it could be moved down to just wrap Lesson instead of the entire app
• It's a class component
  • Consuming hooks is probably not a big deal with HOC like you did with withAnalyticsTracker.
  • To convert to a function component, I think converting setState callback (one which happens right after state is set) is going to be the trickiest part. Big size is contributing to the difficulty as you mentioned
• It's JS
  • It's a contributing factor to slowing down any kind of refactoring with confidence.
  • As you mentioned, big size is one blocker of converting to TS.
  • My suggestion is to still renaming to TS without much changes and suppress or ignore TS errors in the file as a first step. It should be a step forward because we can incrementally fix type, new changes there will be typed. At least it has less concern of breaking things if we just change types and annotations

Let me know your thoughts. Thanks!

[didoesdigital]

Thanks so much for sharing your thoughts here @na2hiro! I appreciate the time you've given this and the effort to include PoC commits. A lot of what you've shared makes sense, and I'll follow up with more concrete thoughts on next steps when I have the head space.

[didoesdigital]

Ok, we can migrate to using a state management library. I'd be open to jotai or zustand. Let's start using jotai.

Yes, the setState callbacks in app.js are the places I expect the most hassle. Some user-facing behaviour changes may be necessary, which I hope to at least make conscious decisions around (rather than accidental changes).

From memory, user settings is read in a few places outside of lessons.

Let's start reducing app.js size and migrating state, then convert to TS file with ignored errors.

I'll be away for a week or so without a computer but when I'm back I'd like to review the code to set/load personal preferences, storage code, and file loading.

In the meantime, any incremental PRs for this issue are welcome.

[na2hiro]

Thanks for checking. Yeah, I'll try to clarify what's going to be changed as much as possible.

To move forward with extracting user settings, I resumed from the second commit of https://github.com/na2hiro/typey-type/commits/jotai. However, as I go further, I feel more that I cannot make so granular commit right now.

• Following your idea, maybe it's better to first reduce prop drilling by using context first to make it easier to change. https://github.com/na2hiro/typey-type/commit/53ea65310da509a01fa3e69564d3f0cdbefbe206 is my first trial of making context for appMethods. We can do appProps and appState similarly.
• Typing of descendant components of App.js would help a lot for confidence after changing how it's wired up. I made #169, and probably will do more if I find opportunity

Please take your time and there's no need to rush. I had 10 days vacation last week and that was plenty of time for fun 😅 I'm going to slow down as well

[didoesdigital]

Just an overall progress update on this issue.

Here are all the PRs progressing this work so far:

• https://github.com/didoesdigital/typey-type/pull/171
• https://github.com/didoesdigital/typey-type/pull/175
• https://github.com/didoesdigital/typey-type/pull/179
• https://github.com/didoesdigital/typey-type/pull/180
• https://github.com/didoesdigital/typey-type/pull/181
• https://github.com/didoesdigital/typey-type/pull/182
• https://github.com/didoesdigital/typey-type/pull/183

The main state remaining in App.js is related to met words, global dictionaries, and lessons. The App.js file is getting to a much more manageable place. It might be time to convert App.js to TS soon, even with lots of ignored or suppressed errors initially. Or maybe just adding more JSDoc annotations.

[evgenyfadeev]

Probably you'd want to move the state management down to the components that are using and updating that state. For example - the lesson state and lesson handling functions could be moved to the Lesson component.

That would reduce the app.js and the amount of prop drilling.

[evgenyfadeev]

@didoesdigital what if there were a custom hook useLesson(), which would internally use jotai atoms?

The hook would return the lesson methods and the lesson state - I'd try to implement that if it sounds reasonable.

If this works, It should be fairly simple - the App.js would be copied to useLesson.js, have the irrelevant code removed and refactor the file a hook using the state defined by jotai atoms, then instead of props in the components or context use the the same values from the hook. Finally, clean up the App.js

[didoesdigital]

Sounds great @evgenyfadeev ! We might need to keep a few pieces out of the lesson code, like the personal/global dictionaries and met words. I would guess converting the setState(…, () => {…}) callbacks might be tricky so look out for that.

Feel free to have a go! Draft/progress PRs are welcome too.

[evgenyfadeev]

@didoesdigital is it ok to use absolute imports? That is instead of import something from '../../utils/something' would be import something from 'utils/something'. I would just use this method if I make a new file, that is without making import changes in many files at once.

We might need to keep a few pieces out of the lesson code, like the personal/global dictionaries and met words.

It does seem that some parts could go to some other state, e.g. metricsState. It could also be split out later though.

I would guess converting the setState(…, () => {…}) callbacks might be tricky so look out for that.

Right, I see. Perhaps some of those callback chains are not really necessary, let's see how it goes.

[didoesdigital]

@evgenyfadeev using absolute imports and introducing them in new files sounds ideal. I vaguely recall running into issues trying to use them in Typey Type in the past, though I cannot remember the details (it might have been an issue with an older version of Create React App or TypeScript, or an interaction with something like Storybook/Jest/Sentry). It's probably fine now. You might need to add a "baseUrl" like "src" to tsconfig.

[evgenyfadeev]

Regarding the absolute imports: the app and the tests are working with absolute imports. I'll check the storybook.

Speaking of the tests - does "yarn test" run all the tests, or yarn test:ui and test:unit do something in addition?

What would be a way to see if sentry is working?

Most likely I'll make some prep PR's first, flattening some setState callback chains.

[didoesdigital]

Yarn test should run all the tests. yarn test:ui runs yarn test with an extra argument to filter it down to only the UI tests, which are slow. yarn test:unit runs yarn test with an extra argument to filter it down only unit tests, which are faster and cover a lot of code.

Because test:ui is slow you might want to just run the faster test:unit while iterating on logic then yarn test:ui or yarn test less frequently and at the end.

Does that all make sense?

[didoesdigital]

Regarding Sentry, I'm not sure of a good option to check the absolute imports are fine in sourcemaps, maybe just put it on a small PR and I'll deploy it and then poke around in the Sentry UI. I don't expect it to be a problem.

[evgenyfadeev]

Sure, I run all the tests before making the PR, I was doing yarn test, yarn test:ui and yarn test:unit. So I'll be just running yarn test, thanks for the explanation. I've made PR #197 for the absolute imports, there is one more #196 where I factored out the speech synthesis function to utils.

[evgenyfadeev]

PR #198 removes part of the setState callback chains and replaces document.getElementById(...).focus() with React state update + useEffect in the TypedText.tsx

If this is ok, I can remove the remaining setState chains using the same way. @didoesdigital

The idea is that code like this.setState(newState, () => this.setupLesson())

Would be replaced with this.setupLesson(newState)

Btw, something to look at: the setupLesson method implemented here will apply the patch newState in the end of the function. This probably makes sense, but it is not how it was done before. (The original logic has the new State applied first and then setupLesson() wourd run).

I have a branch app-js-remove-imperative-focus-flatten-setstate-2, where the order of the patch application is more closely matching the original code, but there I have a bug that the revision lesson would not load any content.