Iframe transport #2
Conversation
| * @param {Event} event | ||
| */ | ||
| function onMessage(event){ | ||
| if (event.data.match(PROTOCOL_NAME + '.request')) { |
There was a problem hiding this comment.
Didn't see an easier alternative to constructing dispatcher for the internal use and analyzing the stringified hash before parsing by dispatcher. Is there any better way?
There was a problem hiding this comment.
hmm for as long as someone won't put PROTOCOL_NAME + '.request' somewhere in a payload
|
|
||
| /** | ||
| * Returns a valid communication protocol for native platform | ||
| * @returns {*|{request: request, response: response}} |
There was a problem hiding this comment.
could you add more description here
| * @param {Event} event | ||
| */ | ||
| function onMessage(event) { | ||
| if (event.data && event.data.protocol === PROTOCOL_NAME) { |
There was a problem hiding this comment.
event.data is used couple of times here, consider caching it in local variable
| */ | ||
| function iframeProtocol () { | ||
| return { | ||
| request: function (execContext, target, method, params, callbackId, async) { |
There was a problem hiding this comment.
what about using object with params instead of 6 function arguments ?
There was a problem hiding this comment.
One todo added with invoke method + ticket for refactoring :)
|
except the missing white space 👍 @hakubo please take a second look :-) |
|
oh right looking good - if works :) |
|
plus could you document it a little how to use it on both sides? thanks |
|
Sure thing, will add an example and documentation to the readme + would love to make a sanity check on both Android and iOS devices if this doesn't spoil anything there ;) |
|
🙇 |
| ((callbackId) ? '&callbackId=' + encodeURIComponent(callbackId) : ''); | ||
| } else { | ||
| throw "Context doesn't support User Agent location API"; | ||
| locationError(); |
There was a problem hiding this comment.
Proxying the exception will report the line inside locationError as the source, I'd suggest to create a LocationException type and throw here, it achieves the same goal but reports the correct line in the source.
|
Good job! I've left a couple inline remarks and I'll keep my fingers crossed for the results of getting this for a ride on real devices ⏩ |
|
Ok, verified as working on iOS, with a little patch for a bug we found (see object.keys commit) ;) |
|
Ok, this appears to work just fine with the apps. I also added a little todo comment for refactoring a little too big number (6) of parameters in invoke method (requires changes in the app repo). @federico-lox @hakubo @RafLeszczynski if you're not waving red flags, i would love to merge it ;) |
|
Hey @bkoval, there are still a couple of requests from the participants to improve both the readme and the inline documentation, I think it's worth addressing those before merging since this is an open source project. Let's make a deal, you address those and we wave a big green flag 😉 |
|
Docs updated, mergin' ;) |
No description provided.