fix: unify file and image types #3050
Conversation
| selectedFiles.some((file) => | ||
| file.id ? file.id === asset.id : file.uri === asset.uri || file.originalUri === asset.uri, | ||
| ), | ||
| selectedImages.some((image) => image.uri === asset.uri) || |
There was a problem hiding this comment.
Why are we doing this change exactly ?
There was a problem hiding this comment.
We don't rely on asset ID or originalUri(which is gone now), which is irrelevant for the new changes and are not supported by the RNFile type anyways. This can be pretty much checked from the file URI only and we can avoid the complex check here.
There was a problem hiding this comment.
We generate an id when we upload an image or file anyways so no point of getting an id from the native API here IMO.
There was a problem hiding this comment.
I'm talking about the file.id specifically. Let's please not introduce such changes (that we haven't tested properly) in larger PRs like this that involve quite a bit of refactoring.
There was a problem hiding this comment.
Since we are not passing ID anymore and RNFile doesn't have a property ID, you can't check for it ideally and talking about uri, this works as far as I have tested.
| const fileUploadsValue = fileUploads | ||
| .map(({ duration, paused, progress, state }) => `${state},${paused},${progress},${duration}`) | ||
| .join(); | ||
| const fileUploadsValue = fileUploads.map(({ state }) => state).join(); |
There was a problem hiding this comment.
Why are we getting rid of the other values here ? I feel like this could break memoization for file uploads, but not sure
There was a problem hiding this comment.
It was vague. I tested it after removal and we don't really need it.
| uri: attachment.asset_url, | ||
| size: attachment.file_size || 0, | ||
| type: attachment.mime_type || '', | ||
| uri: attachment.asset_url || '', |
There was a problem hiding this comment.
Shouldn't we fail if there is no asset_url ?
There was a problem hiding this comment.
Yes, we should, but here, it is almost certain that an attachment would have a URI if it's uploaded because we derive the file uploads from attachments here. I am not sure why the LLC decided to keep these fields as optional. If you have any alternate solution please feel free to propose.
|
Hey @khushal87 , you can also add these changes as well: https://getstream.slack.com/archives/C02EK5GMU8Z/p1743760718807389?thread_ts=1743687681.506999&cid=C02EK5GMU8Z |
The goal of the PR is to unify the File and the Asset type, which was previously vaguely implemented to a common Client supported type,
RNFile. This can be imported directly from the LLC and used in the SDK and by integrators as well.The following are the changes and bug fixes in the PR:
LocalAttachmentpost upload and send the message.FileUploadtype is now a lot simplified compared to what it was before.uploadNewFile; this is for the case where, for audio voice recordings, we do send the type, which cannot be really derived from the mime_type. We can remove it once we finally move to the message composer completely imo.NOTE: The PR is tested on all the upload modes for native CLI/expo android/iOS.