Page MenuHomePhabricator

[web] Use new intial redux state

Authored by michal on Sep 12 2023, 3:33 AM.
Referenced Files
F2086303: D9141.id31067.diff
Sun, Jun 23, 12:36 PM
Unknown Object (File)
Wed, Jun 19, 2:17 AM
Unknown Object (File)
Wed, Jun 12, 8:33 AM
Unknown Object (File)
Mon, Jun 10, 9:29 AM
Unknown Object (File)
Mon, Jun 10, 9:29 AM
Unknown Object (File)
May 21 2024, 5:10 PM
Unknown Object (File)
May 21 2024, 5:10 PM
Unknown Object (File)
May 21 2024, 5:10 PM



Depends on D9140

Use the new action during app load.

Performance note:

  1. When user loads a new version of the app, it should be slightly slower because now they need to do two requests instead of one
  2. If the js is already cached it should be more or less the same

But these changes allow us to do an optimization for case (1) -> we could first download just enough to start rehydration and fetching of the initial redux state, and do it concurrently to the fetching the rest of js. I'm going to create a task for this later because it would require more changes.

Test Plan

Load the web app (logged in), check the redux actions:

  1. @@INIT
  2. persist/PERSIST -> added the _persist field, with rehydrated: false
  3. persist/REHYDRATE -> updated the persisted fields + set rehydrated
  4. SET_NEW_SESSION (it's part of the useServerCall so it runs first) -> updated sessionID
  5. SET_INITIAL_REDUX_STATE -> diff view shows only timestamps updated + initialStateLoaded = true

Also tested reloading when not logged in and the app worked.

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

19 ↗(On Diff #30957)

I don't know if this is still true, but at some point it was better to declare React components using function syntax instead of "arrow function" syntax because the former would have names but the latter wouldn't.

Looks like the V8 in the Chrome doesn't have this problem anymore, though... this advice might be outdated:

Screenshot 2023-09-12 at 2.42.31 PM.png (292×380 px, 31 KB)

19 ↗(On Diff #30957)

A file isn't attached.

Regarding arrow vs. function, in my previous project, we were exclusively using arrows everywhere without any issues. Now I don't have a strong preference, but maybe we should keep consistency if there's no good reason to break it.

ashoat added inline comments.
19 ↗(On Diff #30957)

Sorry – fixed the file attachment issue!

Changed the component to function instead of arrow function. Added the error handling so any errors from the promise will bubble up to ErrorBoundary and display the "Something went wrong, please reload" page. We don't really have a any other good way to handle errors here.

Looks good but please add the dependency array

24 ↗(On Diff #31067)

I prefer to avoid adding explicit values when it's obvious but just my preference so up to you

25–29 ↗(On Diff #31067)

You missed dependency array

32 ↗(On Diff #31067)

seems cleaner

This revision is now accepted and ready to land.Sep 15 2023, 3:19 AM