-
Notifications
You must be signed in to change notification settings - Fork 469
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
Major fixes #59
Major fixes #59
Conversation
CachedImage.js
Outdated
}, | ||
|
||
getDefaultProps() { | ||
return { | ||
renderImage: props => (<Image ref={CACHED_IMAGE_REF} {...props}/>), | ||
renderImage: props => (<ImageBackground ref={CACHED_IMAGE_REF} {...props}/>), |
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 prefer ImageBackground
over Image
?
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.
React recommends to use ImageBackground.
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.
To confirm, using ImageBackground
would require implementors of react-native-cached-image
to update to 0.46? If so and @kfiroo is happy with this, this should probably be denoted as a peerDependency
in the package.json
.
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.
Added alternative in "progressive enhancement" style for ImageBackground facebook/react-native@9637dd4
CachedImage.js
Outdated
@@ -211,6 +219,16 @@ const CachedImage = React.createClass({ | |||
style={activityIndicatorStyle}/> | |||
) | |||
}); | |||
}, | |||
|
|||
renderError() { |
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.
We would probably want to give the users a way to render their own errors.
What do you think? A prop for renderError
?
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.
Good.
@iegik WOW! Thanks for all the good things! I really appreciate your help! :) I absolutely loved the use Any chance you could update the README to reflect your changes? As a side note, this would have been a lot better as separate PRs with a descriptive title so we could reason about it better :) |
|
It`s hard for me split job. I just learning to do that. |
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.
Thanks for the work @iegik. Just a few things I've seen to be looked at but this is much appreciated!
ImageCacheProvider.js
Outdated
// 'Response': { | ||
// Raw: toFile | ||
// } | ||
// }); |
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.
Is this commented code required, and could it be removed before merging?
ImageCacheProvider.js
Outdated
// Error: err | ||
// } | ||
// }); | ||
}) |
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.
Same with this catch that will swallow all errors, unless that is the intention?
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 agree, looks like the comment was supposed to include the catch
clause?
package.json
Outdated
"crypto-js": "3.1.9-1", | ||
"lodash": "4.17.4", | ||
"react-native-clcasher": "git+ssh://git@bitbucket.org/clcorpdevelopment/react-native-casher.git#aa8ead1", |
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.
This should be a version number to the npm package.
@kfiroo I'm weary of this dependency addition.
- It's a private repo
- The NPM package has no readme https://www.npmjs.com/package/react-native-clcasher
It would be good if the required caching functions are included in this library.
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 added following documentation. Will be available in next release.
Currently, we do not expected, that package will available as private. We will fix this later. It`s free.
Also, I think, we should provide flexible caching provider.
MemoryCache
Extended AsyncStorage with expiration check
AsyncStorage can only save data forever. If you want save data for some period of time and clean outdated data -
use following API:
set(key: string, value?: mixed, expires?: seconds)
- Stores data by key and expiration time in secondsget(key: string)
- Returns stored data by keyremove(key: string)
- Clear data by keymultiGet(keys: array)
- Get data by keysmultiSet(values: object, expires?: seconds)
- Store multiple data with expiration time in secondsmultiRemove(keys: array)
- Clears storage by specified keysflush()
- Clear storageisExpired(key: string)
- Checks of data expirationgetAllKeys()
- Returns all stored keysgetAllValues()
- Returns all stored serialized values
Installation
npm install --save react-native-clcasher
Usage
const MemoryCache = require('react-native-clcasher/MemoryCache').default;
MemoryCache.set(url, headers, maxAge)
ImageCacheProvider.js
Outdated
@@ -1,7 +1,8 @@ | |||
'use strict'; | |||
|
|||
const _ = require('lodash'); | |||
|
|||
// const debug = require('console-network'); |
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.
To remove?
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 use my own console-network
tool for debugging Image HTTP request (print in console).
There is delemma:
RNFetchBlob.fetch()
.then(/* Here we can get HTTP response*/)
.catch(/* Here we can get fetch error */)
.then(/* finally */)
// ...
.catch(/* Here we can get file move error */)
How we can provide normal interface for catching fs / fetch errors?
Tested on RN v0.39.2 |
@mattvot Thanks for the review! :) In response to #59 (comment) :
Also, if I got that right, So, to conclude, I think keeping |
@kfiroo I think is too much requests to fs (AsyncStorage, saving to TMP, moving), but it`s seems to be ok. I think we can merge now, or I missed something? |
…-image # Conflicts: # CachedImage.js # CachedImageExample/index.js
renderImage: props => { | ||
return (<Image ref={CACHED_IMAGE_REF} {...props}/>) | ||
}, | ||
renderImageBackground: props => { |
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.
@iegik Why do we need another prop for renderImageBackground
?
I'd rather have the original renderImage
test for ImageBackground
and fallback to Image
for older versions
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.
renderImageBackground
aka ImageBackground
is needed only when we have children
, otherwise we should use old Image
.
children
for images we use for placeholders only.
Is there any status on this PR to be merged? |
@Dvidan I think all issues were resolved with |
@aofeng2009 Please, read carefuly https://github.com/kfiroo/react-native-cached-image/releases/tag/v1.4.0 |
resolveHeaders
property? #58)