Page MenuHomePhabricator

[lib] Create a store in redux for the invite links
ClosedPublic

Authored by tomek on May 19 2023, 9:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 19 2024, 6:44 PM
Unknown Object (File)
Feb 19 2024, 5:12 PM
Unknown Object (File)
Feb 19 2024, 3:36 PM
Unknown Object (File)
Feb 19 2024, 2:12 PM
Unknown Object (File)
Feb 19 2024, 12:02 PM
Unknown Object (File)
Feb 19 2024, 6:04 AM
Unknown Object (File)
Jan 10 2024, 3:20 AM
Unknown Object (File)
Jan 1 2024, 11:47 PM
Subscribers

Details

Summary

A couple of screens require invite links info, and it wasn't an issue until I added an option to update a link - then the data displayed on the screens was inconsistent. The approach with redux should be used from the beginning.

It will also significantly simplify introduction of invite link updates (if we decide to have them).

Depends on D7848

Test Plan

Tested in combination with the rest of the stack: updating a link on one screen and opening another screen where that link is displayed should show the current value.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil requested changes to this revision.May 22 2023, 1:34 AM
kamil added inline comments.
lib/types/link-types.js
39 ↗(On Diff #26674)

This is used twice in this diff, and probably will be used in the future in different places, so I think it will be more readable to make this a separate type, but up to you.

native/redux/persist.js
536 ↗(On Diff #26674)

At this point, the state is not a type of AppState, because it does not have required prop inviteLinksStore.

In other migrations this thing is common so probably it's okay - just it doesn't feel correct to me.

536–542 ↗(On Diff #26674)

are you sure this is needed?

I think we use by default autoMergeLevel1 - so rehydrated state should be merged with the initial state, and this will have no effect anyway.

web/redux/redux-setup.js
100 ↗(On Diff #26674)

It's not optional so you have to add this to the initial state on the web as the client will expect this object to be defined.

This revision now requires changes to proceed.May 22 2023, 1:34 AM
lib/types/link-types.js
39 ↗(On Diff #26674)

This is quite similar to what we do e.g. with threads or messages. But I'll add this new type.

native/redux/persist.js
536 ↗(On Diff #26674)

That's true, but following this convention is convenient.

536–542 ↗(On Diff #26674)

Interesting. I added it after some errors I've seen. I can test and remove if not necessary, but also I'm not sure if spending too much time on it makes sense.

web/redux/redux-setup.js
100 ↗(On Diff #26674)

I guess it's ok - it will be introduced on web soon. On the other hand, we could wait with introducing it on web until the diffs are up - but it shouldn't matter.

native/redux/persist.js
536–542 ↗(On Diff #26674)

I've tested it by:

  1. Checking out a version before this migration was introduced
  2. Wiping out the state
  3. Opening and closing the app
  4. Checking out this version with migration deleted
  5. Opening the app - the store contained {"links": {}} which proves that it works

Delete unnecessary migration

lib/selectors/invite-links-selectors.js
9 ↗(On Diff #26871)

We should avoid returning mutable objects from createSelector calls, as the object is memoized by createSelector, and mutating it will affect future invocations of the selector

kamil added inline comments.
native/redux/persist.js
536–542 ↗(On Diff #26674)

Interesting. I added it after some errors I've seen.

Not sure what exactly errors you saw, but I suspect that it was an alert saying that EXPECTED KEYS NOT REHYDRATED: ["inviteLinksStore"], this should be shown only once and I don't think it's a problem - just logging that there is a new key in the default store, not persisted yet.

web/redux/redux-setup.js
100 ↗(On Diff #26674)

I still do not feel comfortable with the fact, that we create places (like this one in this particular case) where we assume that variable is of a given type but it isn't, but agree, it shouldn't matter

This revision is now accepted and ready to land.May 23 2023, 4:08 PM
native/redux/persist.js
536–542 ↗(On Diff #26674)

No, these were exceptions saying something like "cannot read property links of undefined". But maybe they were connected to requests and not the state.

web/redux/redux-setup.js
100 ↗(On Diff #26674)

Yeah, that's not great, but a possible solution would require a lot of work and maintenance. For one migration it doesn't sound bad: we can have a type without one field. But then, when another migration adds yet another field, we would need three types: one for AppState, one without one field, and one without two. It feels like it's really not worth it... maybe if we frequently encounter bugs caused by these types we could investigate other options, but up to this point I don't remember any accident caused by this.