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

[roryabraham]

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 necolas/react-native-web#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]

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

[melvin-bot]

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

[melvin-bot]

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

[Beamanator]

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]

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

1. Start by adding support for headers - so nothing about supporting POST (yet)
2. 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:

1. Add the ability to handle POST requests - that's the easy part
2. 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]

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]

📣 @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]

@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]

@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]

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

Yes, I'll do that

[Beamanator]

@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]

@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]

Thanks @kidroca ! I triggered an adhoc build for that PR, it should be ready in an hour ish 👍

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.54-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Image Web/Desktop: Add support for http headers #36560
• [CP Staging] fix concierge avatar not showing in preview #38554

If no regressions arise, payment will be issued on 2024-03-26. 🎊

For reference, here are some details about the assignees on this issue:

• @kidroca does not require payment (Contractor)
• @aimane-chnaif requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aimane-chnaif] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[trjExpensify]

Looking pretty good here, no revert! What's next? We move to #9402? :excited:

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.55-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] fix concierge avatar not showing in preview #38554

If no regressions arise, payment will be issued on 2024-03-29. 🎊

For reference, here are some details about the assignees on this issue:

• @kidroca does not require payment (Contractor)
• @aimane-chnaif requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@kidroca / @aimane-chnaif] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[Beamanator]

@trjExpensify do you mind helping figure out payments here so we can close this out? 😅 🙏 I'm not sure who was the previous BugZero person assigned here, but looks like we got none now :O

[trjExpensify]

Yeah, sure. Kidroca is paid separately, so nothing to do there.

@aimane-chnaif is due payment for the C+ review, so I'll send an offer for $500 via Upwork. This issue existed prior to the recent change to $250.

As for the regression test, I don't think this issue warrants Applause testing for "http properties in source".

[trjExpensify]

@aimane-chnaif offer sent! Let me know when you accept.

[trjExpensify]

Paid!