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
[TS Migration] Migrate Image to TypeScript #31296
[TS Migration] Migrate Image to TypeScript #31296
Changes from 8 commits
f67e753
ba4eda8
98bd9e1
1e95421
ed90b35
c1d0c2c
b423c06
46c9f92
0041ba8
b446609
21c7980
f115914
e2508f9
3f260bc
3a76fb0
247dfff
02daaf5
0e11fb8
31aa36a
06c374c
e9451f0
a695adb
ee81360
9ceb2eb
50726c7
4c69801
3f5550f
1056c9f
b13fce2
611227e
7b6ce98
6cc06a2
e24eef6
3b958ba
7ba47c8
403dc95
080dbf4
747f7f5
4851245
2f780c6
9dcfc34
0ef3bf5
171e45a
5d125cf
d1e55cc
1e582b6
065c27c
05919f2
cc78940
c0f10cd
3476ef1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Safe change?
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 tested it and it didn't cause any issues, and also null is not supported
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.
Safe to remove these?
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.
Some time ago we decided to remove these as they are not used anywhere (here)
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.
const dimensionsCache = new Map();
should also be removed - it's not exposed externally and it's only used toset
items internally, but neverget
them. I've raised this subject before and it seems the code and related functionality are leftovers.I've removed the same blocks of code in my RP as well (#36560)
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.
not going to block on this because @kidroca is addressing the
dimensionsCache
in a separate PRThere 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.
This is unrelated to this PR, but the Image will not re-render if styles or other props change.
@kowczarz, Could you share your insights on comparing only the source in the
imagePropsAreEqual
function?This file was deleted.
This file was deleted.
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.
Comment unnecessary, the name of type speaks for itself.
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.
Done
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.
NAB but wouldn't
unknown
make more sense thanvoid
here (and foronLoadEnd
,onError
,onLoad
,onProgress
) since we just don't know or care about the return type?