[lib] Add viewerID to DMOperationSpec.notificationsCreationData
ClosedPublic
Actions

Authored by ashoat on Sep 19 2024, 3:47 PM.

Details

Reviewers

tomek
kamil
will

Commits

rCOMM14c96b9e3566: [lib] Add viewerID to DMOperationSpec.notificationsCreationData

Summary

We need to get viewerID in there, since we need it to populate targetID of UpdateRelationshipMessageData.

Depends on D13390

Test Plan

For now, just Flow. I'll test this at a later point in the stack; for now, putting up diffs following the team's recent pattern to get feedback

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Sep 19 2024, 3:47 PM

ashoat added a child revision: D13392: [lib] Introduce updateRelationshipSpec.

ashoat added inline comments.Sep 19 2024, 3:55 PM

lib/shared/dm-ops/process-dm-ops.js
379–385	Talked to @kamil about this check. Here's what he had to say: Hey, I was thinking about this a bit and my first idea was to put this check here, after generating messages and then return all IDs of generated messages as failed. However, looking deeper, when there is no viewerID we not generate messages anyway so I think we can just return empty array here and should be fine On the other hand, it will be strange case to send composable messages when there is no viewer so I think `invariant` could work too I decided to go with an empty array because I'd rather avoid crashing the app with an `invariant`. Ultimately it's probably a scenario that we never suspect to occur

Harbormaster completed remote builds in B31719: Diff 44349.Sep 19 2024, 4:03 PM

ashoat requested review of this revision.Sep 19 2024, 4:03 PM

LGTM but suggesting alternative approach:

I am also thinking about adding viewerID to ProcessDMOperationUtilities, as it seems like the right place to put it and could simplify it, curious also for @tomek's perspective.

This revision is now accepted and ready to land.Sep 20 2024, 1:41 AM

I am also thinking about adding viewerID to ProcessDMOperationUtilities, as it seems like the right place to put it and could simplify it, curious also for @tomek's perspective.

Yeah, I think it could make sense - we usually use 1 or 3 parameters, so having the viewerID in utils should simplify things a little.

Makes sense, I had the same thought. I'll make that change in a separate diff, as I think it will affect a lot more files

Going to land this now even though it hasn't been tested. This is in keeping with our strategy this month of "land quickly, test later" :)

Closed by commit rCOMM14c96b9e3566: [lib] Add viewerID to DMOperationSpec.notificationsCreationData (authored by ashoat). · Explain WhySep 20 2024, 6:16 AM

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMM14c96b9e3566: [lib] Add viewerID to DMOperationSpec.notificationsCreationData.

ashoat mentioned this in D13412: [lib] Merge viewerID into ProcessDMOperationUtilities.Sep 20 2024, 6:30 AM

In D13391#377106, @kamil wrote:

LGTM but suggesting alternative approach:

I am also thinking about adding viewerID to ProcessDMOperationUtilities, as it seems like the right place to put it and could simplify it, curious also for @tomek's perspective.

D13412

ashoat mentioned this in rCOMM8703bff04d40: [lib] Merge viewerID into ProcessDMOperationUtilities.Sep 20 2024, 7:55 AM