search

idibidiart / react-native-responsive-grid

Bringing the Web's Responsive Design to React Native
Other
379 stars 42 forks source link

Optimize setScreenInfo(). #27

Closed peacechen closed 6 years ago

peacechen commented 6 years ago

Cache mediaSize, etc values and recalculate only if screen dimensions have changed. See #26

peacechen commented 6 years ago

Note that my previous commits for the setState exception show up because my fork was never merged. But fear not because I sync'd with upstream so all your changes are safe.

idibidiart commented 6 years ago

Imagine the situation where setScreenInfo is called with onlySize = true and then it's called with onlySize = undefined || false.... The second call which is expected to return aspect ratio info in addition to size info will get the cached copy of screenInfo and won't have the aspect ratio info. We basically had two optimizations: the onlySize condition which I had from before and the caching which you've introduced. Obviously, the only thing we need is the caching.

if we have both the caching and the condition then you don't really know what you've cached vs what you're expecting.

I've also renamed the last occurrence of cuttOffs to breakPoints. An updated PR is coming.

Thank you very much for this.

peacechen commented 6 years ago

It looks safe to remove the onlySize parameter as long as calls that only need to calculate size (without aspect ratio) don't unnecessarily calculate it. The cached values should prevent that. Hopefully your code has been tested with multiple nested Row, Col, Grid and rotation events :)

The spread operator ..._screenInfo was used precisely for the scenario where setScreenInfo(true) was later followed by setScreenInfo(false). You're right that aspectRatio could end up uninitialized in my PR. Fortunately that's no longer necessary after the removal of onlySize.

There should be a check for the validity of _screenInfo when testing whether to return cached values. https://github.com/idibidiart/react-native-responsive-grid/blob/3227d9a6ffe5ed6fa92c9d40de99599bbdfb5ce3/src/lib/ScreenInfo.js#L71

if ((_screenWidth === SCREEN_WIDTH) && (_screenHeight === SCREEN_HEIGHT) && _screenInfo) {

I don't have a specific scenario where RN returns Dimensions of 0, but if it ever does, setScreenInfo() will return undefined.

idibidiart commented 6 years ago

There should be a check for the validity of _screenInfo when testing whether to return cached values. react-native-responsive-grid/src/lib/ScreenInfo.js

if ((_screenWidth === SCREEN_WIDTH) && (_screenHeight === SCREEN_HEIGHT) && _screenInfo) {

I don't have a specific scenario where RN returns Dimensions of 0, but if it ever does, setScreenInfo() will return undefined.

_screenInfo is an object once the calculation runs. It will evaluate to True regardless of the values in it. I initialize it to null initially for semantic reasons so if RN somehow returns 0 for Width and 0 for Height _screenInfo will be null, likely indicating a bug with RN. I could initialize it to the kind of object you had in your PR but then that'll just push the error further down. I'd rather get an exception like "no such property mediaSize on null" than to proceed with mediaSize of 0.

idibidiart commented 6 years ago

Btw have not tested but I'm comfortable to merge (use the fork Luke!)

peacechen commented 6 years ago

There is a project to render RN in the browser (react-native-web). The container in that case could be 0 size for various reasons. Sometimes web developers use that to hide components, or during resizing it could momentarily go to 0. An easy guard for the initialization case is to init _screenWidth & _screenHeight to -1 instead of 0. Dimensions should never return negative values.