Aaronius / penpal

A promise-based library for securely communicating with iframes via postMessage.
MIT License
389 stars 56 forks source link

Reconnecting if child reloaded #14

Closed jrencz closed 6 years ago

jrencz commented 6 years ago

I noticed that if I reload the page in the iframe the API is not re-established on neither side:

It's easy for the end user to break the experience by triggering child reload (one can always trigger it by using context menu element "Reload Frame" in Chrome for example). It's also easy to break the experience for child: if it has to reload (for either reason) the whole thing just stops working

Aaronius commented 6 years ago

I'm curious to know how you think this should work from a consumer viewpoint.

Should the parent somehow be notified and then forced to call Penpal.connectToChild again? If yes, then I'll probably need to figure out a way to reuse the same iframe when connectToChild is called the second time. If no, then I'll probably need to update the child object upon reconnection since the child could connect to the parent using a different methods object (with different method names) than the first time it connected.

While the connection is lost, Penpal should probably return a rejected promise if the parent calls any child methods.

jrencz commented 6 years ago

The problem is that patent, as far as I know, has no out of the box way to tell if child did reload apart from counting on the fact that child will ping the parent as it loads. Maybe this is the way? I mean sending a ping when child calls its own connectToParent and if parent "thinks" the connthinks has already been established it will know that child did reload.

Apart from restoring the connectivity parent may also register a callback (in the future it may become an observable but since its not a standard yet let's just stick to callbacks?) which will be called each time child connection is re-established. Callback argument should probably be the same as the value connection.promise gets resolved with.

Anyway: the very nature of parent side of connectin IMO implies an observable. Child side is truly a promise because if parent reloads then child is off course loaded again.

jrencz commented 6 years ago

The optimal behavior would be not to have to call connectToChild again in such situation. Being notified that child did reload is a bonus

joshkg commented 6 years ago

Plus one on this feature request! If you can get it done tomorrow that'd be great 😂

Our scenario is that we have an iframe hosting another application, which transitions URLs as part of its flow. Each new URL within the iframe loses connection to the parent and we need to reestablish connection without destroying the iframe.

Aaronius commented 6 years ago

👍 On my TODO list. My plan is that if the child reconnects and exposes different methods than it did on the first connection, the parent's child object that it was initially provided through the promise will be updated with the new methods.

Also, in order to allow the consumer to determine when a reconnection happens, I will allow the consumer to call connection.onReconnect(function(child) { ... }) where the callback will be called upon any reconnection.

Any ideas for improvement are welcome.

joshkg commented 6 years ago

Sounds good, we'll be happy to test it!

Aaronius commented 6 years ago

Alright, I've published this on the next dist-tag. You can try it out via npm install penpal@next. It required me to remove support for allowing parentOrigin to be an array on connectToParent() (previously added in https://github.com/Aaronius/penpal/pull/2). Because that in particular is a breaking change, I'm queueing this up for v3. I did not add a connection.onReconnect() API or any other API that would notify you that a reconnection took place. I'm not at all opposed to it, but I want to understand the use case for it first.

Give it a shot and let me know how it works for you.

Aaronius commented 6 years ago

You can read the documentation on reconnection here: https://github.com/Aaronius/penpal/tree/v3#reconnection

joshkg commented 6 years ago

Excellent! Seems to be working! Will report any bugs I come across as we continue to test. Thanks!

jrencz commented 6 years ago

I checked 3.0.0-alpha.5 and calling methods (either from parent side or from child side) works OK, but lack of reconnection callback/observable will require additional implementation effort in my case:

After the connection is established on parent side the parent sends configuration to the child. Reconnected child obviously won't get this configuration

At the moment I can think of a workaround: reverse the direction of configuration flow. It's not the parent who sends configuration, it's rather the child who requests it. But it feels odd.

jrencz commented 6 years ago

In fact it's not as bad as i thought.

on 2.7.1 in my case the child exposed some methods parent might call after getting connected in order to configure the child. They were called by the parent after the Penpal.connectToChild().promise resolved.

on 3.0.0-beta.5 I added initChild method to the api exposed by parent. Now the parent performs the same configuration steps as it did after connecting but it does them on demand (which means it can do it more than once). Child calls parent.initChild() after Penpal.connectToParent().promise gets resolved.

For the sake of simplicity I'd say it can stay like this until more users report it would be beneficial to have it baked in. Let's just describe the technique I used

Aaronius commented 6 years ago

Yeah, that works. One option would be to change connection.promise to connection.onConnect(() => {...}) or something else that's not promise-based and call the callback function on both the initial connection and every reconnection thereafter.

jrencz commented 6 years ago

Why not both? Promise would work as it used to and onConnect (I'd suggest onConnected BTW) would be called on each connection. That's basically what my solution is yet without a need to call anything in child

Aaronius commented 6 years ago

I would consider onConnected and promise to be redundant. The only use case where they might not be considered redundant is if a user only wants to be notified of the first connection, in which case promise would be a simpler choice. I'm not sure that that use case is a valid one though, and they could achieve the same using onConnected by storing whether their callback was previously called (or converting their callback to a promise).

Aaronius commented 6 years ago

The awkward thing with going with onConnected for the parent is that promise still makes sense on the child since it will be connected at most once. I'd hate to have onConnected on the parent and promise on the child and I'd rather not make the child use onConnected. :/

jrencz commented 6 years ago

I see it this way:

So: if the choice would be to preserve the promise and not add onConnected or to remove it and replace it with onConnected completely I'd vote for preserving the promise and letting the user implement the onConnected feature with methods if it's needed. I see onConnected as a supplementary API, not the main one.

Aaronius commented 6 years ago

Thanks for the discussion. I'll think I'll go in that direction.

Aaronius commented 6 years ago

Published in 3.0.0.