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-26] [$500] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator #35984

Closed
j-piasecki opened this issue Feb 7, 2024 · 31 comments
Assignees
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@j-piasecki
Copy link
Contributor

j-piasecki commented Feb 7, 2024

Name of library: expo-image-manipulator

More context: https://expensify.slack.com/archives/C01GTK53T8Q/p1707293299589539
Discussion here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707293319626329

Details

  • Link to package: https://www.npmjs.com/package/expo-image-manipulator
  • Problem solved by using this package: The library we're using now (@oguzhnatly/react-native-image-manipulator) is a transitive fork of expo-image-manipulator. The original package is more actively maintained and uses expo-modules-core so it should support both, the new and the old architecture.
  • Number of stars in GH: 26.7k
  • Number of monthly downloads: 42,954
  • Number of releases in the last year: 9
  • Level of activity in the repo: high
  • Alternatives: @oguzhnatly/react-native-image-manipulator or any of the other expo-free forks of this library.
  • Are security concerns brought up and addressed in the library's repo?
  • How many dependencies does this lib use that will be brought into our code?

0 – since we already have the Expo modules core.

  • What will the effect be on the bundle size of our code?
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f064b153d795f898
  • Upwork Job ID: 1766142757478912000
  • Last Price Increase: 2024-03-08
@j-piasecki j-piasecki added AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2 labels Feb 7, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

Triggered auto assignment to @mountiny (AutoAssignerAppLibraryReview), see https://stackoverflowteams.com/c/expensify/questions/17737 for more details.

Copy link

melvin-bot bot commented Feb 7, 2024

New Library Review

  • Are all the answers in the main description filled out properly and make sense?
  • Who maintains the library and how well is it maintained?
  • How viable are the alternatives?
  • Should we build it ourselves instead?

Once these questions are answered, start a thread in #engineering-chat, ping the @app_deployers group, and call for a vote to accept the new library. Once the vote is complete, update this issue with the outcome and procede accordingly. Here is a sample post:

Hey @app_deployers,

There is a request to add a new library to App that we need to consider. Please look at this GH and then vote :+1: or :-1: on accepting this new library or not.

GH_LINK

@kowczarz
Copy link
Contributor

kowczarz commented Feb 7, 2024

Hey! I'm Kamil from Software Mansion and I would like to work on this task.

@roryabraham
Copy link
Contributor

@mountiny for some context, we did get New Arch support merged for the fork we're currently using, but it has not yet been published to npm. It took about a year for the PR to be reviewed.

The main reason I support this switch is because we're already using a fork of expo-image-manipulator, so using the original source and the most widely adopted version of this library makes sense.

@kowczarz
Copy link
Contributor

kowczarz commented Feb 8, 2024

What will the effect be on the bundle size of our code?

According to https://bundlephobia.com we will add ~2kB (added ~2.5kB removed ~250B), but since @oguzhnatly/react-native-image-manipulator is based on expo-image-manipulator it seems it's a bug in the tool and the bundle size shouldn't be affected that much.

@mountiny
Copy link
Contributor

mountiny commented Feb 8, 2024

I will do this tomorrow

@mountiny
Copy link
Contributor

mountiny commented Feb 9, 2024

@mountiny mountiny added the Reviewing Has a PR in review label Feb 9, 2024
@jeremy-croff
Copy link
Contributor

I traced back the reason for using this fork to help determine my decisions and found it here:
#6301 (comment)

Not having expo at the time was the main reason for using the fork, and because we now have it, it totally makes sense to use the original package.

@roryabraham
Copy link
Contributor

Want to clarify with whatever C+ gets assigned to this issue that we'll pay a $250 bounty for the review instead of the standard $500

@mountiny
Copy link
Contributor

We have got I believe 6 or 7 👍 from App deployers across GH and Slack and 0 pushback so we can go ahead with the PR @kowczarz

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Feb 19, 2024
@mountiny
Copy link
Contributor

Asked for an update on the PR

@muttmuure
Copy link
Contributor

On staging

Copy link

melvin-bot bot commented Mar 7, 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 7, 2024
@melvin-bot melvin-bot bot changed the title [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator [HOLD for payment 2024-03-14] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.48-0 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-14. 🎊

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

  • @kowczarz does not require payment (Contractor)

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Mar 8, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-03-14] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator [$500] [HOLD for payment 2024-03-14] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator Mar 8, 2024
Copy link

melvin-bot bot commented Mar 8, 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.

@laurenreidexpensify
Copy link
Contributor

Payment Summary:

Contributor: @kowczarz no payment, agency contributor
C+ Review: @mananjadhav $500 please request in newdot

@mananjadhav
Copy link
Collaborator

@laurenreidexpensify this issue is not closed. The earlier PR was reverted, and the new one is under review right now. Can you please remove the payout summary and reopen the issue?

@mountiny mountiny changed the title [$500] [HOLD for payment 2024-03-14] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator [$500] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator Mar 15, 2024
@mountiny
Copy link
Contributor

this was reverted so reopening

@mountiny mountiny reopened this Mar 15, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 19, 2024
@melvin-bot melvin-bot bot changed the title [$500] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator [HOLD for payment 2024-03-26] [$500] [New architecture] Replace @oguzhnatly/react-native-image-manipulator with expo-image-manipulator Mar 19, 2024
Copy link

melvin-bot bot commented Mar 19, 2024

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

@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

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:

  • @mananjadhav requires payment through NewDot Manual Requests
  • @hungvu193 requires payment (Needs manual offer from BZ)
  • @kowczarz does not require payment (Contractor)

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:

@laurenreidexpensify
Copy link
Contributor

Payment Summary:

Contributor: @kowczarz no payment, agency contributor
C+ Review: @mananjadhav $500 please request in newdot

@hungvu193 pls apply for payment in upwork - https://www.upwork.com/jobs/~0155bf7bf36c1319ef

@melvin-bot melvin-bot bot removed the Overdue label Mar 26, 2024
@hungvu193
Copy link
Contributor

Payment Summary:

Contributor: @kowczarz no payment, agency contributor

C+ Review: @mananjadhav $500 please request in newdot

@hungvu193 pls apply for payment in upwork - https://www.upwork.com/jobs/~0155bf7bf36c1319ef

I've just applied!

@PierreAdel
Copy link

PierreAdel commented Mar 26, 2024

Upwork Proposal: Upgrading React Native Image Manipulator Package

Dear Team,

To address the issue with the current library, I propose upgrading to the original package, expo-image-manipulator, which is more actively maintained and supports both the new and old architecture. By transitioning to this package, we ensure compatibility with Expo modules core and benefit from ongoing updates and enhancements.

Implementation Plan:

  1. Assessment: Evaluate the current usage of @oguzhnatly/react-native-image-manipulator in our application and identify any potential dependencies or conflicts.
  2. Migration: Replace the existing library with expo-image-manipulator in our codebase, ensuring compatibility with both the old and new architecture.
  3. Testing: Conduct thorough testing to validate the functionality and performance of the image manipulation features with the new library.
  4. Deployment: Once testing is successful, deploy the updated version of the application to production, ensuring a seamless transition for end-users.

Looking forward to it.

Contributor details
Your Expensify account email: pierre@expensify.com
Upwork Profile Link: https://www.upwork.com/freelancers/pierreadel2

Copy link

melvin-bot bot commented Mar 26, 2024

📣 @PierreAdel! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@JmillsExpensify
Copy link

$500 approved for @mananjadhav based on summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests