Skip to content

fix: fixing logic for resizing image url - #1123

Merged
vishalnarkhede merged 4 commits into
v4.0.0-beta.7from
vishal/resize-image-url-fix
Feb 17, 2022
Merged

vishalnarkhede merged 4 commits into
v4.0.0-beta.7from
vishal/resize-image-url-fix

Conversation

@vishalnarkhede

@vishalnarkhede vishalnarkhede commented Feb 3, 2022 •

Copy link
Copy Markdown
Contributor

🎯 Goal

Fixing the logic for resizing the image urls:

  • We can't rely on existence of wildcards to detect if image is resizable, since these wildcards won't be sent from backend anymore. Existing URLs will still contain these wildcards. Thus only thing we need to check is weather image belongs to cloundfront CDN - .stream-io-cdn.com
  • Adding url parser library to ensure we modify the url correctly. Simply appending/replacing has a potential to easily mess up the url - react-native-url-polyfill
  • Added missing tests for getResizedImageUrl function

@vishalnarkhede
vishalnarkhede requested review from khushal87 and madsroskar and removed request for khushal87 and madsroskar February 3, 2022 16:43
@vishalnarkhede
vishalnarkhede marked this pull request as draft February 3, 2022 16:47
@vishalnarkhede
vishalnarkhede marked this pull request as ready for review February 4, 2022 08:20
@madsroskar

Copy link
Copy Markdown
Contributor

I have two concerns regarding the URL polyfill included:

  1. It looks like Expo support was added recently, and only Expo v44 is mentioned in the commit history. I'm going to test it with older versions and see if I notice any issues, but I don't know if you tested it out on older versions already?
  2. The above ☝🏻 isn't included in the most recently released version, as the lib hasn't been updated in the npm registry in the last year (last publish in march last year). Could this be an issue? Is it safer for us to keep a fork we can publish as needed?

@madsroskar

Copy link
Copy Markdown
Contributor

charpeni/react-native-url-polyfill#366 We should probably keep a reference for this for when we work on web support as well

@vishalnarkhede

Copy link
Copy Markdown
Contributor Author

@madsroskar do you know any better library btw? I didn't put too much thought into it honestly

…act-native into vishal/resize-image-url-fix

# Conflicts:
#	package/package.json
#	package/yarn.lock
@vishalnarkhede
vishalnarkhede changed the base branch from develop to v4.0.0-beta.7 February 17, 2022 23:49
@vishalnarkhede
vishalnarkhede merged commit 1d8f8db into v4.0.0-beta.7 Feb 17, 2022
@vishalnarkhede
vishalnarkhede deleted the vishal/resize-image-url-fix branch February 17, 2022 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants