Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
roryabraham opened this issue Nov 9, 2022 · 118 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Nov 9, 2022

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.

@roryabraham roryabraham added the Weekly KSv2 label Nov 9, 2022
@Beamanator Beamanator added the External Added to denote the issue can be worked on by a contributor label Nov 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 10, 2022

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 10, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 10, 2022

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

@melvin-bot melvin-bot bot changed the title RNW Image: Add support for http properties in source [$250] RNW Image: Add support for http properties in source Nov 10, 2022
@Beamanator Beamanator removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 10, 2022
@Beamanator
Copy link
Contributor

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
Copy link
Contributor

kidroca commented Nov 10, 2022

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 puneetlath added NewFeature Something to build that is a new item. Weekly KSv2 and removed Daily KSv2 labels Nov 10, 2022
@puneetlath
Copy link
Contributor

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
Copy link

melvin-bot bot commented Nov 10, 2022

📣 @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
Copy link
Contributor

@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
Copy link
Contributor

@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

@JmillsExpensify JmillsExpensify changed the title [$250] RNW Image: Add support for http properties in source [$250] [Feature] RNW Image: Add support for http properties in source Nov 11, 2022
@kidroca
Copy link
Contributor

kidroca commented Nov 11, 2022

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

Yes, I'll do that

@Beamanator
Copy link
Contributor

Beamanator commented Nov 11, 2022

@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
Copy link
Contributor

kidroca commented Nov 14, 2022

@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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Feb 15, 2024
@Beamanator
Copy link
Contributor

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

Copy link

melvin-bot bot commented Mar 19, 2024

⚠️ 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 melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 19, 2024
@melvin-bot melvin-bot bot changed the title [$250] [Feature] RNW Image: Add support for http properties in source [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source Mar 19, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 19, 2024
Copy link

melvin-bot bot commented Mar 19, 2024

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

Copy link

melvin-bot bot commented Mar 19, 2024

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:

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

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

Copy link

melvin-bot bot commented Mar 19, 2024

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
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 22, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source [HOLD for payment 2024-03-29] [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source Mar 22, 2024
Copy link

melvin-bot bot commented Mar 22, 2024

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:

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

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

Copy link

melvin-bot bot commented Mar 22, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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

@trjExpensify trjExpensify added Daily KSv2 and removed Weekly KSv2 labels May 3, 2024
@trjExpensify
Copy link
Contributor

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests