This allows us to get rid of some of the branching covered in https://linear.app/comm/issue/ENG-6070/tidy-up-flow-branching-from-minimal-permissions-work.
Details
Details
Flow, things continue to work as expected on native and web clients in my local environment.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/reducers/thread-reducer.js | ||
|---|---|---|
| 98–101 ↗ | (On Diff #35450) | I thought putting an invariant here was a reasonable trade-off vs. refactoring the types in the fullStateSyncActionType payload (specifically StateSyncFullActionPayload which is derived from ClientFullStateSync which is derived from BaseFullStateSync) which are used across clients and keyserver. Open to re-exploring if that would be preferred. |
| 275–278 ↗ | (On Diff #35450) | Similarly thought this was a reasonable trade-off to avoid having to refactor types related to state check mechanism. Open to re-exploring if that would be preferred. |
Comment Actions
What's the significance of [DRAFT] here? Should I hold off on review? If so, can you please hit "Plan Changes" (or use Phabricator's "draft" feature in the future)?
Comment Actions
Was going to try to see if I could do some more flow refactoring to remove the two invariants that were introduced to thread-reducer, but figured it'd be good to put up what I had that resolved all flow issues.
That said, think it's a reasonable trade-off to leave the two invariants in thread-reducer.
Comment Actions
Prior to ENG-6138, doesn't the ThreadStore on web actually contain a mix of minimally-encoded and legacy RawThreadInfos?
It seems like if we want to sequence this one prior to ENG-6138, we'll need to separate out the types of ThreadStore between web and native. Might be easier to just do the web migration first. Let me know if I'm missing something
| lib/reducers/thread-reducer.js | ||
|---|---|---|
| 98–101 | This seems scary, but I assume you've made sure that the threadInfos returned by all four of these actions are always minimally-encoded. Is this guaranteed on the keyserver side? I wonder if we could avoid this invariant (and the one highlighted in my later inline comment) by differentiating the client and keyserver version of these types. If the client only ever deals with minimally-encoded variants, we could reflect that in the typesystem. What do you think? | |
| 224 | This is the same variable as threadInfo. I realize this was the case before your changes, but would you mind deduping these two variables? | |
| 274–277 | Also feels scary – same questions as above | |
| 390 | Nit: this could be move to above the currentUser declaration, and then used to slightly simplify that declaration | |
| 393 | Nit: we could use the currentUser variable here | |
| lib/shared/updates/join-thread-spec.js | ||
| 39 | We have a separation between ClientUpdateInfo and ServerUpdateInfo that we've used in the past to avoid invariants like the one below. The two types are currently set as aliases, but I don't think it would be too hard to update ClientUpdateInfo so that it exclusively uses minimally-encoded RawThreadInfos, while ServerUpdateInfo can continue dealing with the union. What do you think? I think that this should only take 5min or so of work, but open to having it done in a separate diff | |
| lib/shared/updates/update-thread-spec.js | ||
| 31–34 | Same question as in my inline comment on join-thread-spec.js | |
| web/types/redux-types.js | ||
| 16 | Can you explain why this one is special-cased? I thought that all keyserver endpoints were returning minimally-encoded RawThreadInfos to all clients that support them | |
Comment Actions
It seems like if we want to sequence this one prior to ENG-6138, we'll need to separate out the types of ThreadStore between web and native. Might be easier to just do the web migration first. Let me know if I'm missing something
We will always get MinimallyEncoded* data in Redux when we pull data from SQLite whether or not the data in SQLite is MinimallyEncoded*
Specifically, this is handled in convertClientDBThreadInfoToRawThreadInfo:
Comment Actions
address nits
| lib/reducers/thread-reducer.js | ||
|---|---|---|
| 98–101 | Yeah, it would indicate something is very wrong if we ever got LegacyTheadInfos from the keyserver on current/new clients. As I mentioned in earlier comment wrt refactoring flow types:
It looks like this is possible, but it would require a lot of changes to socket-types since a lot of types are derived from other types and there are corresponding validators. Would also require changes to inflight-requests and socket.react. There are already Client* and Server* types. I was thinking the cleanest way to do this may be by introducing LegacyClient* types? I didn't spend too much time on this since at the time it seemed like it'd get pretty involved, but I can spend more time coming up with more concrete solution if that'd be preferred to invariant. | |
| 274–277 | Similarly would require lot of refactoring to types involved in state check mechanism. After skimming through the code it seemed like it would be pretty involved so I opted for invariant, but I can go ahead and try coming up with more concrete solution if that'd be preferred to the invariant. Would prefer to do that in a separate diff to avoid this one getting too complex. | |
| lib/shared/updates/join-thread-spec.js | ||
| 39 | We discussed this in our 1:1 on Friday and concluded invariant made sense. I don't remember the exact details, but I'll try making the change you're suggesting. | |
| web/types/redux-types.js | ||
| 16 | This was a poor naming choice. The use of Legacy* doesn't match the meaning of Legacy* as it related to minimally encoded permissions. This is more about ThreadStore that could contain Legacy or MinimallyEncoded threads. I'll find a new name for this. | |
Comment Actions
@atul I want to reemphasize the importance of documenting in-person conversations on Phabricator / Linear. If you had documented our initial conversation, we probably wouldn't have needed to have a second one. Seeing as you didn't document the second one either, I went ahead and did it for you to avoid the possibility of a third conversation.
| lib/shared/updates/join-thread-spec.js | ||
|---|---|---|
| 44–47 ↗ | (On Diff #35507) | @atul and I had a brief discussion about this invariant as well as the one in update-thread-spec.js. I asked him to document the discussion here, but it looks it hasn't been done, so I guess I'll do it. The issue is that the UpdateSpec have a single type parameter for the UpdateInfo that is used for both keyserver methods and client methods. If we split the types between the two of those in order to avoid this invariant, then we'll need to update every single UpdateSpec so that they all have another type param. We decided that that much work wasn't worth it. |
Comment Actions
Saw you planned changes, but I had some comments queued – figure I might as well share them now.
There are already Client* and Server* types. I was thinking the cleanest way to do this may be by introducing LegacyClient* types? I didn't spend too much time on this since at the time it seemed like it'd get pretty involved, but I can spend more time coming up with more concrete solution if that'd be preferred to invariant.
I'm not sure why we'd need to introduce LegacyClient* types, except as intermediate types that are not exported. Not sure if that's what you mean, but ultimately we should be able to stick with just the Client / Server types dichotomy in terms of what's exported from that file.
| web/types/redux-types.js | ||
|---|---|---|
| 16 ↗ | (On Diff #35507) | You described this as a poor naming choice for LegacyThreadStore, but my initial confusion was in regards to why a type in web reflects the possibility of legacy RawThreadInfos being delivered to its store. The naming really didn't factor into it, although I agree that the naming can be improved. I did some investigation on my own and realized that this type in web should actually probably be in lib, as it's used by keyserver as well. The fact that the type is used by keyserver is almost certainly why you need to type it this way. I think we should do two things here:
What do you think? Open to having you do this in a separate diff, but it should take 5min so I feel strongly it should be done right away, and not deferred. |
Comment Actions
The invariants in thread-reducer were removed later in the stack. Specifically in D10609 and D10624.
The update-spec invariants were considered acceptable:
The task involving splitting InitialReduxStateResponse is here: https://linear.app/comm/issue/ENG-6516/split-initialreduxstateresponse-into-client-and-server-variants
I will put up a diff for this shortly, want to rebase and land existing stack first since there are some merge conflicts I need to resolve etc.