Page MenuHomePhabricator

[keyserver/lib] Set up the endpoints and responder for retrieving media
ClosedPublic

Authored by rohan on Feb 2 2023, 9:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 2:12 AM
Unknown Object (File)
Sun, Nov 10, 8:46 PM
Unknown Object (File)
Thu, Nov 7, 11:46 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Subscribers

Details

Summary

This diff handles setting up the endpoints and responder for actually fetching the media for a given thread. The changes made are:

  1. Set up the endpoint (keyserver/src/endpoints.js) and the type (lib/types/endpoints.js)
  2. Use callServerEndpoint in lib/actions/thread-actions.js to hit the newly created endpoint, and route to the appropriate responder.
  3. Handle the validation of the request and call fetchMediaForThread from the responder in keyserver/src/responders/thread-responders.js

Linear: https://linear.app/comm/issue/ENG-2874/set-up-the-endpoints-and-responder-for-retrieving-media

Depends on D6485

Test Plan

Ran flow for all of the new type introductions, and this will be tested in the next diff where we actually use useServerCall to retrieve the media.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Feb 2 2023, 9:21 AM
ashoat requested changes to this revision.Feb 2 2023, 11:00 AM
ashoat added inline comments.
keyserver/src/fetchers/upload-fetchers.js
120 ↗(On Diff #21866)

What is the point of this change?

lib/types/thread-types.js
442 ↗(On Diff #21866)

Don't think we need a separate type for this

This revision now requires changes to proceed.Feb 2 2023, 11:00 AM
keyserver/src/fetchers/upload-fetchers.js
120 ↗(On Diff #21866)

You're right, I don't need viewer here so I can remove it from fetchMediaForThread. I'll do that in the revision

lib/types/thread-types.js
442 ↗(On Diff #21866)

That's fair, I changed this just before putting up this revision to follow the general convention. I can remove both ThreadFetchMediaResult and FetchThreadMediaPayload types and just replace them directly with $ReadOnlyArray<Media> in the method headers.

Remove ThreadFetchMediaResult and FetchThreadMediaPayload and replace them with $ReadOnlyArray<Media> directly. Also remove the unused viewer parameter

rohan planned changes to this revision.Feb 2 2023, 3:06 PM

Looks like some commits got combined, will fix this

Hopefully fix the git issue?

ashoat requested changes to this revision.Feb 3 2023, 10:50 AM

You should keep ThreadFetchMediaResult – otherwise it will be ready hard to update the result to include something new in the future, since it would break older clients

Looks like some commits got combined, will fix this

Encourage you to think critically about your workflow. This sort of thing generally shouldn't happen unless you're being "messy" with git

This revision now requires changes to proceed.Feb 3 2023, 10:50 AM

Bring back ThreadFetchMediaResult, I realize that it definitely makes sense to make future changes (if needed) much simpler

ashoat requested changes to this revision.Feb 3 2023, 8:15 PM
ashoat added inline comments.
lib/types/thread-types.js
438 ↗(On Diff #21971)

Otherwise it's hard to extend this later to eg. return something else as well

This revision now requires changes to proceed.Feb 3 2023, 8:15 PM

Update types and add support for optional param limit

keyserver/src/fetchers/upload-fetchers.js
123 ↗(On Diff #22032)

Changing return type here now that ThreadFetchMediaResult to make it easier to change what's returned if the type is modified

ashoat added inline comments.
lib/actions/thread-actions.js
173 ↗(On Diff #22032)

I would just return response here and have the return type be ThreadFetchMediaResult. It's not as important as the return type from the server (since the server will need to support old clients in perpetuity), but it would make it more convenient for your _SUCCESS action's payload to be an object, which will make it easier to extend that payload in the future without having to update reducer code. It also lets you pass the same object through the whole workflow, which is arguably more convenient

lib/types/thread-types.js
443 ↗(On Diff #22032)

I think limit won't be enough for paginating... you'll probably need a cursor of some sort as well. Not sure if pagination is intended to be addressed later – bringing this up here since you've presumably started work on it given the limit parameter

This revision is now accepted and ready to land.Feb 6 2023, 7:09 AM
lib/actions/thread-actions.js
173 ↗(On Diff #22032)

That's definitely fair, I can just return response here and in the gallery component extract the media from the response

lib/types/thread-types.js
443 ↗(On Diff #22032)

I was planning on using something like OFFSET to prevent loading every single media all at once. This way I could fetch more media on scroll with a specific offset (say 30 images each time) onEndReached

Address feedback, make limit required and introduce offset. Also to prevent passing in more and more parameters to fetchMediaForThread, I changed it to just accept request: ThreadFetchMediaRequest

keyserver/src/fetchers/upload-fetchers.js
126–127 ↗(On Diff #22160)

Nit: I would just inline these in query rather than using .append

Inline offset and limit statements

Check CI failures log and try to fix them

Try to resolve CI failures

Resolve any type now that ThreadFetchMediaResult and ThreadFetchMediaRequest are introduced, allow ThreadFetchMediaResult to
return an adjustedOffset (see previous diff for context), and allow ThreadFetchMediaRequest to accept an array of media IDs (string).