useImageDimensions #87

Greg-Bush · 2020-03-18T22:35:49Z

Summary

This hook will solve the problem when you need to know the original image dimensions or aspect ratio for some complicated markup based on that image. And you might don't know what kind that image will be like: an asset or remote.
Depending on the "source" parameter type it uses one of the following methods:
https://reactnative.dev/docs/image/#resolveassetsource
https://reactnative.dev/docs/image/#getsize

Test Plan

Usage example in readme

Compatibility

OS	Implemented
iOS	✅
Android	✅
Web	❌

Checklist

I have tested this on a device and a simulator
I added the documentation in README.md
I updated the typed files (TS and Flow)
I've created a snack to demonstrate the changes: https://snack.expo.io/@gregbush/useimagedimensions-hook

src/useImageDimensions.ts

LinusU · 2020-03-18T22:44:04Z

Have you considered returning an object with { dimensions, loading, error } instead of having there error as a callback, and the dimensions be undefined when loading. Sort of like how useQuery from Apollo works, or useFetch? If yes, why made you not go that route?

LinusU · 2020-03-18T22:45:45Z

Web | ❌

Doesn't Image.getSize work in react-native-web? 🤔

Greg-Bush · 2020-03-19T00:06:38Z

Have you considered returning an object with { dimensions, loading, error } instead of having there error as a callback, and the dimensions be undefined when loading. Sort of like how useQuery from Apollo works, or useFetch? If yes, why made you not go that route?

Nice idea. Will do.

Greg-Bush · 2020-03-19T00:09:13Z

Web | ❌

Doesn't Image.getSize work in react-native-web? 🤔

React Native for web, what? oO Ok then, it should work I guess.

pvinis · 2020-03-19T09:48:14Z

This is a nice idea! ~~Would you like to make a quick snack too and add it on the PR description?~~

pvinis · 2020-03-19T10:21:41Z

Why did this not produce a canary release? 🤔

Edit: It's because this PR comes from a fork. I enabled the builds to run on PRs from forks, which is great for checking if tests and builds are green, but the canary release doesn't happen because of credentials. I guess it's alright for now, but I created #90 to see if we can enable that.

src/useImageDimensions.ts

LinusU · 2020-03-19T11:54:07Z

src/useImageDimensions.ts

+ * @param source either a remote URL or a local file resource.
+ * @returns original image width and height.
+ */
+function useImageDimensions(source: ImageRequireSource | URISource) {


How about importing ImageSourcePropType from react-native and using that as the type for source?

Suggested change

function useImageDimensions(source: ImageRequireSource | URISource) {

function useImageDimensions(source: ImageSourcePropType) {

There is a lot of redundant properties in ImageSourcePropType too, that are not using in the hook.

Is there any downside to using ImageSourcePropType, even though it has some more props defined though? 🤔

Here is about ImageSourcePropType:
https://reactnative.dev/docs/image#source
This prop can also contain several remote URLs, specified together with their width and height and potentially with scale/other URI arguments. The native side will then choose the best uri to display based on the measured size of the image container.
It means that ImageSourcePropType contains redundant types. No sense to use these types in this hook.

src/useImageDimensions.ts

Greg-Bush · 2020-03-20T19:34:33Z

This is a nice idea! ~~Would you like to make a quick snack too and add it on the PR description?~~

Hey! What is a snack? What do you mean?

LinusU · 2020-03-20T19:49:44Z

A snack is a way to easily share React Native code, mainly used for testing/demonstrating.

Everyone can create one at this site: https://snack.expo.io

It would be great if you could create an example that uses your hook there so that we could see it in action! 💪

pvinis · 2020-03-21T12:08:50Z

But actually I don't know how this could work, since there is no way for @Greg-Bush to make a canary, right?

Greg-Bush · 2020-03-28T19:32:57Z

Hey guys! I have made a snack. The Expo snack is working badly with remote images. I don't know the reason. But I have already tested that hook for mobile in the real project locally and it works ok.
I'm not sure that it will work on the web. Because I don't know how to test that.
Also, I took into account all the comments, and I guess this is the final version, so please consider to merge.

src/useImageDimensions.ts

LinusU · 2020-03-30T09:51:47Z

src/useImageDimensions.ts

+         */
+        readonly aspectRatio: number
+        width: number
+        height: number


How about making width and height readonly as well?

Unlike 'aspectRatio', actually, these fields are not read-only.

I think it could be good to mark them as readonly so catch a potential error where the end user accidentally modifies them, since that will be reset each render loop. What do you think?

I don't understand what do you mean and why do you think that field change will cause anything?

src/useImageDimensions.ts

LinusU · 2020-03-30T14:10:47Z

@Greg-Bush would you mind not pressing "Resolve" after answering comments? It becomes very hard for me to find your replies. Thanks! ☺️

Greg-Bush · 2020-03-30T23:59:53Z

@Greg-Bush would you mind not pressing "Resolve" after answering comments? It becomes very hard for me to find your replies. Thanks! ☺️

Ok, clear

Greg-Bush · 2020-04-14T16:27:50Z

Hey guys! Is there any decision about that pull request?

LinusU

There are still a few things that I would like to be addressed ☺️

src/index.ts

src/useImageDimensions.ts

LinusU · 2020-04-14T17:33:33Z

src/useImageDimensions.ts

+ * @param source either a remote URL or a local file resource.
+ * @returns original image width and height.
+ */
+function useImageDimensions(source: ImageRequireSource | URISource) {


Is there any downside to using ImageSourcePropType, even though it has some more props defined though? 🤔

src/useImageDimensions.ts

This reverts commit 2c2c44e.

Greg-Bush · 2020-04-16T17:59:44Z

@pvinis, the request has long been open. Please, decide here to merge this PR, or not.

LinusU · 2020-04-16T20:16:52Z

Pushed a commit that typed the return value of the type, reused the return value as to not cause unnecessary re-renders, and cleaned up a bit. Landing now! Thanks for you contribution! 🌟

pvinis · 2020-04-16T20:25:08Z

🚀 PR was released in v2.5.0 🚀

Greg-Bush · 2020-04-16T20:39:33Z

Wow! Wtf? Why do you approve your changes and merge them without code review?

Greg-Bush

Useless change. No sense here at all.

LinusU · 2020-04-16T22:06:53Z

Why do you approve your changes and merge them without code review?

I approved your change, and then I made a few changes as I landed. If you want to I'm sure that @pvinis can review them for me... I didn't think that this was controversial since I've seen it done a lot, and also other maintainers merges minor changes in this repo without someone else approving...

Useless change. No sense here at all.

Would you mind elaborating on what part was useless or not making sense? I'd be happy to update...

If you are talking about re-using the return value, I do not think that it's useless since it will make sure that e.g. the following code doesn't re-calculate every time:

const dimensions = useImageDimensions(...)
const foo = useMemo(() => /* compute with dimensions */, [dimensions])

// ...

Greg-Bush · 2020-04-16T22:39:22Z

Ok, clear with "result".

Greg-Bush · 2020-04-16T22:40:57Z

src/useImageDimensions.ts

-  get aspectRatio() {
-    return this.width / this.height
-  }
+export type Source = ImageRequireSource | URISource


Why do you make one more type and export it?

If someone wants to build a hook that extends on this hook, it's nice to be able to have one type that represents any input. That way it can easily be used as the input value to a function and just passed on, e.g:

function useHeaderHeight (logo: Source, background: Source): number { // ... }

Greg-Bush · 2020-04-16T22:42:53Z

src/useImageDimensions.ts

-  }
+export type Source = ImageRequireSource | URISource
+
+export interface ImageDimensions {


Why do you also export this one?

If someone wants to build a hook that extends on this hook, it's nice to be able to have one type that represents the return value. That way it can easily be used as the return value to a function and just passed on, e.g:

function useLogoDimensions (): ImageDimensions { return useImageDimensions(globalSettings.logoImage) }

TypeScript calculates return types automatically, there is no need to mark it manually.
Also, the 'ImageDimensions' name doesn't fully represent the content of this interface. There are 'error' and 'loading' fields. And the sizes themselves are even in a nested property 'dimensions', this field may be called 'ImageDimensions'. The whole object, which contains all of these - not really.

I'd be open to renaming it ImageDimensionsResult or something similar...

TypeScript calculates return types automatically, there is no need to mark it manually.

Many style guides mandate that the return type be specified to avoid accidentally not retuning what you intended by clearly stating the intent up front. I think that's a good practice 👍

tuantranailo · 2020-07-09T17:17:13Z

src/useImageDimensions.ts

+    } catch (error) {
+      setResult({error, loading: false})
+    }
+  }, [source])


A question here: if the source is an object, wouldn't it be changed every render, triggering the warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or **one of the dependencies changes on every render.**? This is because useEffect hook does shallow comparison, not deep comparison. @Greg-Bush and @LinusU

It depends on how you pass in the source, as long as the same object is being passed each render cycle it will not trigger the useEffect to re-run. The one case I can see would be problematic is if people pass the url as such:

useImageDimensions({ uri: 'https://...' })

We could help with this by doing something like:

- }, [source]) + }, [source?.uri ?? source])

@LinusU Yes. That would definitely do the trick. 👍 👍 👍

Greg-Bush added 6 commits March 7, 2020 21:14

useImageDimensions.ts added

cd68287

useImageDimensions added to index.ts

2c2c44e

useImageDimensions API

b4899b7

useImageDimensions anchor

acf539a

useImageDimensions failure param is optional now

2137de9

[useImageDimensions] variable renamed in README

7e40f17

LinusU reviewed Mar 18, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

[useImageDimensions] refactoring

45887f4

pvinis added minor Increment the minor version when merged release Create a release when this pr is merged labels Mar 19, 2020

pvinis added 2 commits March 19, 2020 11:42

Merge branch 'master' into master

5dc0b60

Merge branch 'master' into master

c693037

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Show resolved Hide resolved

LinusU reviewed Mar 19, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

Greg-Bush added 2 commits March 27, 2020 00:57

Merge branch 'master' of https://github.com/Greg-Bush/hooks

651f7d7

[useImageDimensions] minor change

3aac61c

LinusU reviewed Mar 30, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 30, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 30, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

LinusU reviewed Mar 30, 2020

View reviewed changes

src/useImageDimensions.ts Outdated Show resolved Hide resolved

[useImageDimensions] refactoring

6e70840

LinusU requested changes Apr 14, 2020

View reviewed changes

Greg-Bush added 3 commits April 15, 2020 21:58

Revert "useImageDimensions added to index.ts"

a4fb485

This reverts commit 2c2c44e.

Merge remote-tracking branch 'upstream/master'

51d848f

useImageDimensions added to index.ts

7e1b9ab

[useImageDimensions] Reuse return value between renders

20f9833

Fix linting

5c6256b

LinusU approved these changes Apr 16, 2020

View reviewed changes

LinusU merged commit 7bb9837 into react-native-community:master Apr 16, 2020

pvinis added the released This issue/pull request has been released. label Apr 16, 2020

Greg-Bush commented Apr 16, 2020

View reviewed changes

tuantranailo reviewed Jul 9, 2020

View reviewed changes

	function useImageDimensions(source: ImageRequireSource \| URISource) {
	function useImageDimensions(source: ImageSourcePropType) {

useImageDimensions #87

useImageDimensions #87

Conversation

Greg-Bush commented Mar 18, 2020 • edited Loading

Summary

Test Plan

Compatibility

Checklist

LinusU commented Mar 18, 2020

LinusU commented Mar 18, 2020

Greg-Bush commented Mar 19, 2020

Greg-Bush commented Mar 19, 2020

pvinis commented Mar 19, 2020 • edited Loading

pvinis commented Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greg-Bush commented Mar 20, 2020

LinusU commented Mar 20, 2020

pvinis commented Mar 21, 2020

Greg-Bush commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusU commented Mar 30, 2020

Greg-Bush commented Mar 30, 2020

Greg-Bush commented Apr 14, 2020

LinusU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greg-Bush commented Apr 16, 2020

LinusU commented Apr 16, 2020

pvinis commented Apr 16, 2020

Greg-Bush commented Apr 16, 2020

Greg-Bush left a comment

Choose a reason for hiding this comment

LinusU commented Apr 16, 2020 • edited Loading

Greg-Bush commented Apr 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuantranailo Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greg-Bush commented Mar 18, 2020 •

edited

Loading

pvinis commented Mar 19, 2020 •

edited

Loading

pvinis commented Mar 19, 2020 •

edited

Loading

LinusU commented Apr 16, 2020 •

edited

Loading

tuantranailo Jul 9, 2020 •

edited

Loading