Merges in both debugging and ensuring fetch is cancelled#22
Merges in both debugging and ensuring fetch is cancelled#22RobertFischer wants to merge 7 commits into
Conversation
seekshiva
left a comment
seekshiva
left a comment
There was a problem hiding this comment.
Hey Robert,
This is a really good PR. Thanks.
Can you take a look at these review change requests?
| import { View, WebView } from 'react-native'; | ||
| import { View, WebView, StyleSheet } from 'react-native'; | ||
| const fetchingDebug = require("debug")("react-native-remote-svg:SvgImage:fetch"); | ||
| const fetchResultDebug = require("debug")("react-native-remote-svg:SvgImage:fetch:result"); |
There was a problem hiding this comment.
Hey, it looks like you used the debug package to debug this issue. Does it make sense to commit it onto master?
| "prop-types": "^15.6.2" | ||
| }, | ||
| "devDependencies": { | ||
| "supports-color": "^5.4.0" |
There was a problem hiding this comment.
This package doesn't seem to be used anywhere. Was this required for the use of debug package? Can this be removed?
| const props = this.props; | ||
| const { svgContent } = this.state; | ||
| if (svgContent) { | ||
| const props = this.props || {}; |
There was a problem hiding this comment.
this.props should always be an object in react. Have you seen any situation where it might be falsy? It shouldn't be
| const { svgContent } = this.state; | ||
| if (svgContent) { | ||
| const props = this.props || {}; | ||
| const { svgContent } = this.state || {}; |
There was a problem hiding this comment.
We are initializing state for this component. So this || {} wouldn't be necessary.
| .call("text") | ||
| .then(text => this.setState({ svgContent: text })) | ||
| .catch(e => console.error(`Error fetching SVG URI: ${e.message||e}`, {uri, e})) | ||
| .return((previousFetch && previousFetch.isPending()) ? previousFetch : null) // Ensure we resolve/cancel previous fetch |
There was a problem hiding this comment.
Shouldn't we cancel the previousFetch if the uri has changed?
| @@ -0,0 +1,2 @@ | |||
| yarn.lock | |||
There was a problem hiding this comment.
I think it is actually recommended for yarn.lock to be committed, since multiple people developing the project would all be on the same locked version.
2 participants
#20 and #21 wrapped up into one nice package. :)