Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
useImageDimensions #87
useImageDimensions #87
Changes from 21 commits
cd68287
2c2c44e
b4899b7
acf539a
2137de9
7e40f17
45887f4
5dc0b60
c693037
d0f7e0a
2424139
d3bdf8e
ad73da5
ca71be9
651f7d7
3aac61c
6e70840
a4fb485
51d848f
7e1b9ab
20f9833
5c6256b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you make one more type and export it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you also export this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be open to renaming it
ImageDimensionsResult
or something similar...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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 becauseuseEffect
hook does shallow comparison, not deep comparison. @Greg-Bush and @LinusUThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theuseEffect
to re-run. The one case I can see would be problematic is if people pass the url as such:We could help with this by doing something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LinusU Yes. That would definitely do the trick. 👍 👍 👍