Fix the vector web version in UserSettings #542
Conversation
Add a getAppVersion() function to the platform rather than relying on the updater code firing an event before we know what the app version is.
| * the current version of the application. | ||
| */ | ||
| getAppVersion() { | ||
| return q(); |
There was a problem hiding this comment.
how about making this throw an error, so that people don't forget to implement it?
| @@ -167,7 +167,6 @@ export default React.createClass({ | |||
| case PageTypes.UserSettings: | |||
| page_element = <UserSettings | |||
| onClose={this.props.onUserSettingsClose} | |||
| version={this.props.version} | |||
There was a problem hiding this comment.
needs removing from UserSettings.propTypes
| }; | ||
| }, | ||
|
|
||
| componentWillMount: function() { | ||
| if (PlatformPeg.get()) { | ||
| PlatformPeg.get().getAppVersion().done((appVersion) => { | ||
| this.setState({ |
There was a problem hiding this comment.
needs a guard against being unmounted before response
| }; | ||
| }, | ||
|
|
||
| componentWillMount: function() { | ||
| if (PlatformPeg.get()) { | ||
| PlatformPeg.get().getAppVersion().done((appVersion) => { |
There was a problem hiding this comment.
is there a situation in which getAppVersion thows an exception, and takes out react? worth sticking a q().then in front of this?
There was a problem hiding this comment.
done()'s error handler is equivalent to this
There was a problem hiding this comment.
no, it's not.
done will handle rejected promises. I'm talking about exceptions thrown synchronously by getAppVersion.
|
ptal |
|
more comments or is this good? |
|
See #542 (comment) |
in case it throws
Add a getAppVersion() function to the platform rather than relying
on the updater code firing an event before we know what the app
version is.
Requires element-hq/element-web#2553