Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconnecting if child reloaded #14

Closed
jrencz opened this issue Oct 23, 2017 · 18 comments
Closed

Reconnecting if child reloaded #14

jrencz opened this issue Oct 23, 2017 · 18 comments

Comments

@jrencz
Copy link

jrencz commented Oct 23, 2017

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

  • in parent there's no event to listen to in order to call Penpal.connectToChild again (even if one should call it manually)
  • in child, even though the page starts again it can't connect to already existing parent

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
Copy link
Owner

Aaronius commented Oct 25, 2017

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
Copy link
Author

jrencz commented Oct 25, 2017

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
Copy link
Author

jrencz commented Oct 25, 2017

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
Copy link

joshkg commented Nov 8, 2017

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
Copy link
Owner

Aaronius commented Nov 8, 2017

👍 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
Copy link

joshkg commented Nov 8, 2017

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

@Aaronius
Copy link
Owner

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 #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
Copy link
Owner

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

@joshkg
Copy link

joshkg commented Nov 10, 2017

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

@jrencz
Copy link
Author

jrencz commented Nov 13, 2017

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
Copy link
Author

jrencz commented Nov 13, 2017

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
Copy link
Owner

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
Copy link
Author

jrencz commented Nov 13, 2017

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
Copy link
Owner

Aaronius commented Nov 14, 2017

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
Copy link
Owner

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
Copy link
Author

jrencz commented Nov 15, 2017

I see it this way:

  • promise has a cool feature of being able to expose other side methods when they are available: whether it already happened or it will happen. In this matter it's way more convenient than the callback. It also can be passed around to retrieve methods from everywhere
  • on the other hand it's not the tool to notify about things that happen more than once
  • keeping Penpal.connectToChild().promise will keep the 3.0.0 compatible with previous version (I'd say there's no need to remove it if its not necessary)
  • not every use case will make use of onConnected being called more than once. Something like this is required in some use cases. Most probably very few since up until now it was not possible at all.

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
Copy link
Owner

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

@Aaronius
Copy link
Owner

Published in 3.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants