-
Notifications
You must be signed in to change notification settings - Fork 40
Conversation
b7d51d2
to
c850655
Compare
c850655
to
407fcba
Compare
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.
Looking good so far!
|
||
let jsonItem = try self.parser.jsonStringFromItem(item) | ||
|
||
return self.webView.evaluateJavaScript("\(self.dataStoreName).touch(\(jsonItem))") |
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… don't… understand… this code.
Doesn't evaluateJavascript
return void
? Oh, god it, Rx monkey patches.
self = date | ||
} else { | ||
return nil | ||
} |
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.
Nit: Could you document some of the sample strings this is expected to handle?
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.
Does the test count as documentation here? Or are you saying that I could include more cases in the spec file to be more thorough?
.map { items in | ||
guard let id = item.id else { | ||
return items | ||
} |
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.
Ok.
@@ -68,4 +68,14 @@ class SwiftInteropDataStore extends DataStoreModule.DataStore { | |||
} | |||
}) | |||
} | |||
|
|||
async touch(item) { | |||
return super.touch(item).then( function(updatedItem) { |
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.
Nit: Use
updatedItem => {}
instead of function
.
resolves #152