[HOLD for payment 2024-03-29] [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source

roryabraham commented 1 year ago

Problem:

Adding headers to images works in React native, but when this gets converted to Web & Desktop code (via react-native-web) it doesn't work - see https://github.com/necolas/react-native-web/issues/1019

Solution:

Fix this, since we're planning to start sending encryptedAuthToken in chat report image requests via header!

cc @kidroca since you've accepted the challenge to take this on.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

melvin-bot[bot] commented 1 year ago

Current assignee @Beamanator is eligible for the External assigner, not assigning anyone new.

Beamanator commented 1 year ago

Removing Help Wanted since @kidroca is gathering some thoughts & plans to get going on this. I'll assign him once he posts in this issue 😅

kidroca commented 1 year ago

Hey guys, the linked ticket actually had a PR and it looks like it was almost merged: https://github.com/necolas/react-native-web/pull/1470/files

As you can see it change a bit too much

There are a few things that I'd like to do differently

Start by adding support for headers - so nothing about supporting POST (yet)
Don't change ImageUriCache (yet) - the existing PR uses everything in the ImageSoruce to create unique key for cache, but this is incorrect IMO - headers of a GET request should not matter for cache (same as browser cache - if you make the same GET request with different headers you get results from cache (when available))

Then continue with anything else that has to be implemented:

Add the ability to handle POST requests - that's the easy part
Decide whether react-native-web should cache those - this makes a likely unnecessary job hard - as you know POST requests aren't typically cached - they aren't considered idempotent. The easiest thing would be to just not attempt to cache those

So that's the brief plan

puneetlath commented 1 year ago

I added the NewFeature label since I believe this is new functionality, not a bug with existing functionality. And I demoted to weekly as a result. @Beamanator If that's incorrect, let me know!

melvin-bot[bot] commented 1 year ago

📣 @kidroca You have been assigned to this job by @Beamanator! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Beamanator commented 1 year ago

@kidroca thanks for the outline of your plan, sounds great to me 👍 I also don't understand why we would need to cache post requests (or if it's a good idea) so it sounds like that's something we don't need to worry about, they can implement that themselves if they want 🤷

Just to be clear, will you make a PR in the main project and in our fork?

Beamanator commented 1 year ago

@puneetlath that makes sense to me 👍 It's technically a bug, but we haven't used that functionality in the past so it didn't affect us - so it's kinda 1/2 1/2 :D

kidroca commented 1 year ago

Just to be clear, will you make a PR in the main project and in our fork?

Yes, I'll do that

Beamanator commented 1 year ago

@thomas-coldwell & I have been chatting a bit about this issue, @kidroca do you know if this fix would also work if we used react-native-fast-image in Web / desktop code? a.k.a. do you think react-native-web would treat Image components from react-native similar to the Image component from react-native-fast-image?

Wondering b/c we think it would be nice to use react-native-fast-image everywhere in the App (assuming we can get it to work w/out any issues / limitations) so the codebase is a bit more consistent & readable. Does that make sense to you too?

kidroca commented 1 year ago

@Beamanator This might work if react-native-fast-image entry point exports different code by platform

android -> Glide
ios -> SDWebImage
others -> fallback to react-native Image (webpack transforms react-native imports to react-native-web

Actually it seems there's a flag about it: https://github.com/DylanVann/react-native-fast-image#fallback-boolean When fallback is true FastImage would use react-native Image , for example:

<FastImage fallback={Platform.os == 'web'} />

It would be best to have explicit support for web - react-native-fast-image might list -web as a peer dependency and use it, if at some point they need more control over stuff that's encapsulated in -web they can create their custom component (and stop using react-native-web )

I think we can easily test this theory by creating a custom App Image component with multiple exports

index.native.js -> <FastImage {...props} />
index.js -> <FastImage fallback {...props} />

If that works out ok, we can leave it like that or we can push a few changes for -fast-image to do the same behind the scenes

Beamanator commented 1 year ago

I think we can easily test this theory by creating a custom App Image component with multiple exports

index.native.js -> <FastImage {...props} />

index.js -> <FastImage fallback {...props} /> If that works out ok, we can leave it like that or we can push a few changes for -fast-image to do the same behind the scenes

Ok nice so if this works, we could potentially make a pretty simple Image wrapper component that has the files you mentioned ^, yeah? And maybe as a Part 2, submit a PR to react-native-fast-image to get web working (maybe with the same strategy).

I'm just thinking we can get it working ASAP in App, then we can clean up later. I think it's better to implement the fix in App quick instead of waiting for a PR to get merged in react-native-fast-image or to make a local fork of that repo

Beamanator commented 1 year ago

So @kidroca it seems like we are going with this step by step plan:

You're making web & desktop cache images by fixing react-native-web (this issue)
@thomas-coldwell is upgrading iOS (& maybe Android) caching via react-native-fast-image (https://github.com/Expensify/App/pull/12648)

Once these are implemented, "caching images should work". Then we can work on:

Using react-native-fast-image everywhere (which means using fallback prop for Web / Desktop)
Clear out / crush related issues in the image enhancements tracker: https://github.com/Expensify/App/issues/10894

kidroca commented 1 year ago

Yep, all good

thomas-coldwell commented 1 year ago

If you can give me a ping when the RNW side of things is sorted @kidroca I'll go through and ensure support using fast image across the whole app as per @Beamanator comment 🙌

kidroca commented 1 year ago

This is my PR, when it's reviewed I'll open the same PR for the react-native-web repo

https://github.com/Expensify/react-native-web/pull/8

I've stumbled on 2 issues that we'd need to address to make this work in App

ImageWithSizeCalculation

This component uses the getSize static method that works with only uri - no source with headers https://github.com/Expensify/App/blob/9a8291ba97c381b651b5344d6fa471c51ae70336/src/components/ImageWithSizeCalculation.js#L70

I've made the onLoad event to return the image size like it works in RN so we can use that (Behind the scenes getSize downloads the image to read the dimensions, so in the case of ImageWithSizeCalculation where we also load/render the image at the same time - it just makes 2 parallel requests for the same image uri, using the onLoad event would be better as it'll get the size and load the image with just 1 request)

CORS issues on localhost

Access to fetch at 'https://www.expensify.com/?exitTo=%2Fchat-attachments%2F949802747%2Fw_1426cdf2308f4ec256096c4dc216242423efdada.jpg' (redirected from 'http://localhost:8080/chat-attachments/949802747/w_1426cdf2308f4ec256096c4dc216242423efdada.jpg') from origin 'http://localhost:8080' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

I get this error when I make a request with headers - perhaps this case is not handled by the local proxy, or there's something else...

This is the request, that then gets redirected and triggers the CORS error

fetch("http://localhost:8080/chat-attachments/949802747/w_1426cdf2308f4ec256096c4dc216242423efdada.jpg", {
  "headers": {
    "accept": "image/*",
    "accept-language": "en-US,en;q=0.9,bg;q=0.8,es;q=0.7",
    "sec-ch-ua": "\"Google Chrome\";v=\"107\", \"Chromium\";v=\"107\", \"Not=A?Brand\";v=\"24\"",
    "sec-ch-ua-mobile": "?0",
    "sec-ch-ua-platform": "\"Windows\"",
    "sec-fetch-dest": "empty",
    "sec-fetch-mode": "cors",
    "sec-fetch-site": "same-origin",
    "x-chat-attachment-token": "ZEDTsRyteH%2BTYlHCVIOV1CajPvWJn2LuwA6%2FIOT78QLRVyPSNVa4oHYU7V84kK0cTVcr3gaMHuUOrLEzVU9OhoHKwh68eBXUOqXp4nRGmBPTGFIPcpHXbwPHdr7DMd2RidVI37OzTK2%2BdtR%2FjzgK2TOc7qx4IcMB7ylDb0hTc%2B%2F3VcPl%2F01USx82%2B72thlU7NatoWaXmLY7gftIZLKpapRg%2BtF5ritEjGrg7eViYQS8VudT7JzFrip%2F8jd6BFXLJqW1A6RlRR8Ow1Ku1GfRcqKMxG0N5mIA5hoI22AabBVNF%2FzL8sWyuKTBFpt3UOl5iBfN%2FVfFuCVrb1XE86YeuxekKl%2FnlIfz3HfpbVqFXhjS7JZzVq9HirP%2BEticCOfCnq9yKGC1vKwRjzoGxGFOSsWZ%2Fs%2FkC48S%2FAPid%2FFQeMxezigHUeCCXQsSR8j%2FdmnIN4oa1kI66VR%2BQXQonKaOuDTnMVYVFtyCxoLPr6pmz7Nuwu6mO2QYqvGNhLH0CZhsFCT6DdFWklMN%2BUxEA%2F0LkqQq0UiqSVkVRA0O5q4gez%2B4Dj3rrhIuxRBd2D%2FT4de%2FpKCidYccNa%2B%2BZOkfVDyc4ObcQDtckfxJP2liaiXKzK8CEd%2B2KEdXGHftXSBrLuUnNlvTBbEd8Tqq8KBFktsSzAqEUSoOYnrtaDgUChSmFAvsqmH6E1vj8OTbcbcgsnZT7%2BZmjIjpMfcElzHB1OtiI%2FZPXpaueQhV2e7FWKMHek7rDMuWq%2BMDpta8mVe9GqPyDbBScMFrHjVaZ5u8i0OlKQQkWopn6QHkHa6e263i5S7gWLlOxo5hPlVakAuG28%2F5qeN%2Fx0roiB20kTsBUI74%2Fw0e8YnSkVIGYzh%2FHEcMaQ6LU0jhrTHnpBSijLrH1iOfBjdeevvViwK%2BwF1aFE%2Bg7nqV2WrA4cyaQR4fkGHXvmIEdUdH4axszcJSMvrH1SUmc%2BbZiLaNsttPCCKohcNRxpQ%3D%3D"
  },
  "referrer": "http://localhost:8080/r/71955477",
  "referrerPolicy": "strict-origin-when-cross-origin",
  "body": null,
  "method": "GET",
  "mode": "cors",
  "credentials": "include"
});

Judging by the ?exitTo on the redirected request, there might be annoter error at play and the CORS issue is just a coincidence

Beamanator commented 1 year ago

Super pumped about this, sadly I haven't had time to review today (catching up on lots of things while i was OOO & the brain is super foggy from intense jetlag) - I'll definitely get to this tomorrow, sorry for the delay!

Beamanator commented 1 year ago

We've been discussing a bit in this thread about how to test & what we'll update in an App PR

rushatgabhane commented 1 year ago

Alright, I kinda understand what is happening at the high level but not enough to be useful in reviews.

cc @parasharrajat you wanna give this a look?

kidroca commented 1 year ago

Opened a few PRs:

for Expensify fork of RNW - https://github.com/Expensify/react-native-web/pull/8
for mainstream RNW repo - https://github.com/necolas/react-native-web/pull/2442
for App - https://github.com/Expensify/App/pull/13036 This one showcases Image requests with headers working for Web/Desktop.

kidroca commented 1 year ago

Using react-native-fast-image everywhere (which means using fallback prop for Web / Desktop)

I've tried that, but because react-native-fast-image tries to load some native modules in it's init script it's not possible to import it on Web (or at least I couldn't easily find a way) So it seems we can't just use the fallback prop I think we should just use the RNW Image component for web/desktop

I did manage to get Images with headers load on web, so we can move forward with what we've got

necolas commented 1 year ago

Fix this, since we're planning to start sending encryptedAuthToken in chat report image requests via header!

How would you expect to do this if image loading worked as it does natively on the web, i.e., just props on <img>?

Beamanator commented 1 year ago

@necolas I believe in this case we'd try to go with the approach of using fetch to send the image get request, with the auth token in a header, then setting the response as the image's source - similar to what's described here. I see there's a few cons though, but they seem unconfirmed

I see you mentioned here that there's "numerous other plans around Image" for your react-native-web library, and I see that the issue we're trying to solve (https://github.com/necolas/react-native-web/issues/1019) is part of your milestone (https://github.com/necolas/react-native-web/milestone/18). Do you prefer solving this issue after other issues in the milestone? Or is there another reason you mentioned you'd prefer we contacted you before making the PR?

Beamanator commented 1 year ago

Making progress on upstream PR: https://github.com/necolas/react-native-web/pull/2442

JmillsExpensify commented 1 year ago

Looks like that PR has stalled a bit?

kidroca commented 1 year ago

Looks like that PR has stalled a bit?

Yes, I'm waiting for maintainer feedback/review for the PR on the main repo

necolas commented 1 year ago

The PR hasn't "stalled". I'm busy working through dozens of other changes for RN and RNWeb that are going to be part of the 0.19 release and were planned months before that PR was posted.

JmillsExpensify commented 1 year ago

@necolas I'm super sorry! I didn't mean for that comment to come across as it unfortunately did. We're super appreciative of what you've done with react-native-web, and my comment came from us being eager to help move things forward. If there is any way we can improve how we collaborate from here on out, we're very open to your thoughts and suggestions. It's admittedly been a balance managing upstream fixes that benefit everyone versus our own fork. Thanks again!

Beamanator commented 1 year ago

@kidroca In order to unblock our other image enhancement issues, here's our new plan:

Work on getting https://github.com/Expensify/react-native-web/pull/8 merged into our fork
Get that update working in our App (which will unblock numerous other issues around image caching)
Open a new issue tracking the upstream PR so when it gets merged, we can pull those changes into our fork

Can you let me know if the fork PR is ready to be tested in App? 🙏

kidroca commented 1 year ago

Allright @Beamanator Since I had to squash my changes to fix conflicts in the original repo, I'll have to do the same for Expensify's fork

Can you let me know if the fork PR is ready to be tested in App? 🙏

The logic is already established and we verified it here: https://expensify.slack.com/archives/C01GTK53T8Q/p1669800005882449?thread_ts=1669058284.677789&cid=C01GTK53T8Q But I'll have to apply the latest changes (the squash) to be in sync with the original repo

The PR we used as a POC: https://github.com/Expensify/App/pull/13036 is using code that was reverted In order to make my work testable in App, I'll need that same code available again, maybe it already is - I'll post back if I have any difficulties otherwise I should be done by tomorrow

Beamanator commented 1 year ago

The PR we used as a POC: https://github.com/Expensify/App/pull/13036 is using code that was reverted In order to make my work testable in App, I'll need that same code available again, maybe it already is - I'll post back if I have any difficulties otherwise I should be done by tomorrow

@kidroca it looks like you're talking about https://github.com/Expensify/App/pull/12648 which got reverted, right? There's a draft follow-up here: https://github.com/Expensify/App/pull/13304 (which also involves a PR to react-native-fast-image here: https://github.com/DylanVann/react-native-fast-image/pull/953)

kidroca commented 1 year ago

@Beamanator I've updated the PR to our fork with latest code I've also cherry picked everything I need as a single commit for the PR for App here: https://github.com/Expensify/App/pull/13036 This way we can test the Image web/desktop logic in App - when the other PR (fast-image) is ready I'll drop the "cherry commit" and use the actual code, but for web/desktop there should be no difference

My only problem right now is that I can't login - getting 403s - I've submitted my email and IP to be whitelisted

Beamanator commented 1 year ago

Thanks @kidroca 👍

Sadly I'm actually going OOO till Jan 1, maybe @roryabraham could you help test this out before I get back, or maybe @marcaaron ?

kidroca commented 1 year ago

Reposting my last summary as it's still valid (https://github.com/Expensify/App/issues/12603#issuecomment-1327517367)

Opened a few PRs:

for Expensify fork of RNW - https://github.com/Expensify/react-native-web/pull/8 - this is ready for review ✔️
for mainstream RNW repo - https://github.com/necolas/react-native-web/pull/2442 - I don't think this is fully reviewed yet
for App - https://github.com/Expensify/App/pull/13036 - ATM this is a draft showcasing Image requests with headers working for Web/Desktop.

I guess someone needs to review the PR for our fork so we can merge it in order to start using it in App

https://github.com/Expensify/react-native-web/pull/8

Beamanator commented 1 year ago

Thanks @kidroca - sorry for the delay, I was OOO for a bit over the holidays.

I just did a review on the Expensify fork - overall it looks great but it would be great to get a few things cleared up (some in GH comments, others in code comments) before we move forward - I'm also asking Marc to re-review & see if we need another set of 👀

kidroca commented 1 year ago

I've addressed PR comments and we're close to a final version of the PR

kidroca commented 1 year ago

I've verified the latest PR changes are still working OK in App, though I did discover this CORS issue by accident: https://expensify.slack.com/archives/C049HHMV9SM/p1673289214225929

It seems that because an attachment was sent from STAGING it needs to be loaded from the STAGING url (even though I'm on a different ENV)

This + HEADERS qualifies the request as a CORS request and creates an issue (because the backend response does not satisfy CORS rules)

I think it would be best if we can skip the origin from the chat attachment URL, this would make the request against the current origin, so instead of saving https://staging.expensify.com/chat-attachments/...atachment.jpg we save just /chat-attachments/...atachment.jpg and use that as source or at least add the prefix/origin ourselves

Beamanator commented 1 year ago

Little updates on this issue:

Fork PR was merged 🎉
App PR was updated and should be ready for review - cc @aimane-chnaif
Upsteam PR has some activity - @kidroca when you get a chance can you please respond to the latest comment? It looks like the maintainer is asking for more tests

kidroca commented 1 year ago

Upsteam PR has some activity - @kidroca when you get a chance can you please respond to the latest comment? It looks like the maintainer is asking for more tests

I need to sync the latest changes to the upstream PR, and write some unit tests

MelvinBot commented 1 year ago

This issue has not been updated in over 15 days. @puneetlath, @Beamanator, @kidroca eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

puneetlath commented 1 year ago

@Beamanator I'm going to unassign myself from this since I don't think we need a BZ on this one. Feel free to assign me back if I'm wrong about that.

kidroca commented 1 year ago

The Upstream PR is updated and it's ready for review

Meanwhile the current task is blocked by

https://github.com/Expensify/App/issues/14991

Should we update the PR and issue title with [HOLD] ?

trjExpensify commented 1 year ago

Yeah, I think we should update the title of this issue if it's blocked: [Hold - #14991]

johnmlee101 commented 1 year ago

Do we believe this is a dupe? https://github.com/Expensify/App/issues/15922 where the loading of attachments on the viewer is blocked by the caching this implements?

kidroca commented 1 year ago

Do we believe this is a dupe? https://github.com/Expensify/App/issues/15922 where the loading of attachments on the viewer is blocked by the caching this implements?

I don't think so

The spinner probably appears because we render it until the onLoad event is fired

Even when we have image in cache it would still fire this event

(e.g, Our image component always starts from loading state)

(I'll double check and post back)

kidroca commented 1 year ago

@johnmlee101 I've double checked and we're not a blocker - images are still getting cached for at least 2hrs An image loads from cache if we go back to it

MelvinBot commented 1 year ago

This issue has not been updated in over 15 days. @Beamanator, @kidroca eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Beamanator commented 1 year ago

This one's still on hold but maybe we can keep it Monthly since we're holding on a bigger project

trjExpensify commented 1 year ago

Can we link to the right place for the hold on this issue, please? The issue title references #14991 which has merged, so it's confusing as to what the dependency really is. Thanks!

Expensify / App