Details

Reviewers

bartek
tomek
inka
michal
ashoat

Commits

rCOMM28cbe2cf3e82: [lib] Add containedThreadInfos selector

Summary

Part of ENG-4319.

A new selector is needed to obtain contained chats. There exists childThreadInfos selector, but it operates on parentThreadID rather than on containingThreadID.

Depends on D8686.

Test Plan

Tested later in the stack.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• patryk created this revision.Jul 18 2023, 3:53 AM

• patryk held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 18 2023, 3:53 AM

Harbormaster completed remote builds in B20985: Diff 28770.Jul 18 2023, 4:11 AM

• patryk edited the summary of this revision. (Show Details)Jul 18 2023, 7:01 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added reviewers: bartek, tomek, inka, kamil.

• patryk edited the summary of this revision. (Show Details)Jul 18 2023, 7:03 AM

• patryk added a child revision: D8530: [web] Warn the user when deleting a chat that has contained chats.Jul 18 2023, 7:35 AM

• patryk edited reviewers, added: michal; removed: kamil.

• patryk published this revision for review.Jul 19 2023, 1:05 AM

inka accepted this revision.Jul 19 2023, 3:11 AM

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

bartek accepted this revision.Jul 19 2023, 4:24 AM

bartek added inline comments.

lib/selectors/thread-selectors.js
212–214 ↗	(On Diff #28770)	Do we rely on the difference between `null` and `undefined` here? IIRC we do so below in `result[containingThreadID]`

• patryk added a child revision: D8590: [native] Warn the user when deleting a chat that has contained chats.Jul 21 2023, 7:41 AM

ashoat added inline comments.Jul 27 2023, 9:48 AM

lib/selectors/thread-selectors.js
203 ↗	(On Diff #28770)	Would be best to avoid `*` here, which is basically `any` in modern versions of Flow. What happens if you try using `mixed`? Alternately, what happens if you make the following type updates: Update `lib/types/redux-types.js` to specify a default value for the type param, eg. `export type BaseAppState<NavInfo: BaseNavInfo = BaseNavInfo>` Use the default type param here: `BaseAppState<>`
211 ↗	(On Diff #28770)

• patryk added inline comments.Jul 31 2023, 12:49 AM

lib/selectors/thread-selectors.js
203 ↗	(On Diff #28770)	Error occurs: `Cannot instantiate BaseAppState because mixed is incompatible with BaseNavInfo in type argument NavInfo [Flow(incompatible-type-arg)]` We get prop missing, incompatible exact and variance errors: property ... (e. g. activeChatThreadID) is missing in BaseNavInfo but exists in NavInfo; property startDate is read-only in BaseNavInfo but writable in NavInfo; BaseNavInfo is incompatible with exact NavInfo in property navInfo;

Unpack containingThreadID property.

Harbormaster completed remote builds in B21350: Diff 29246.Jul 31 2023, 1:29 AM

ashoat mentioned this in D8686: [lib] Replace BaseAppState<*> with default type param.Jul 31 2023, 11:34 AM

I don't have much spare time, but I took a look at this for you

This is happening because navInfo is not read-only in AppState. This is exactly why we try to make everything read-only when possible. More details on this general class of problems here.

Please rebase your diff onto my stack: D8686

Then take out the *

This revision now requires changes to proceed.Jul 31 2023, 11:39 AM

Rebase and remove *.

• patryk edited the summary of this revision. (Show Details)Aug 1 2023, 12:59 AM

• patryk added a parent revision: D8686: [lib] Replace BaseAppState<*> with default type param.

Harbormaster completed remote builds in B21444: Diff 29406.Aug 1 2023, 1:16 AM

ashoat mentioned this in rCOMMc451a8155e79: [lib] Replace BaseAppState<*> with default type param.Aug 1 2023, 6:27 AM

Thanks!

This revision is now accepted and ready to land.Aug 1 2023, 6:28 AM

• patryk retitled this revision from [web] Add containedThreadInfos selector to [lib] Add containedThreadInfos selector.Aug 2 2023, 3:44 AM

Rebase