Feature suggestion: parent/child connection timeout should reject promise

[jrencz]

As far as I can see if I run child sand-alone and I make an attempt to connect to the parent the promise stays pending indefinitely.

It's super-easy to detect "just" running stand-alone in child (by checking if window.parent !== window), but it becomes harder if connection was not properly established (for any reason in fact)

Currently I use Promise.race for that but it seems like a reasonable thing to have it baked in.

[Aaronius]

Throwing an error when connectToParent is called and the child is not in an iframe sounds reasonable. I'd just throw an error rather than returning a connection object. Regarding rejecting a promise if the connection cannot be established, a few thoughts come to mind:

Would you expect the promise to reject if the connection weren't established after a certain amount of time (potentially configurable)? Would you expect the same for connectToChild? Would you expect the same for all method calls?

[jrencz]

Ok: throwing an error outside of an iframe in case connectToParent is called seems reasonable since it can be checked synchronously.

As I mentioned I added an opt-out in my test implementation and that's IMO what users should probably be encouraged to do in README if child is operational at all when loaded as stand-alone.

But this way it's definitely a breaking change. It's easy to think about already existing pages using Penpal exploiting the fact that calling connectToParent is currently a noop if there's no parent.

Would you expect the promise to reject if the connection weren't established after a certain amount of time (potentially configurable)?

Yes. That's exactly what my Promise.race-based implementation does.

Would you expect the same for connectToChild?

That actually seems to be far more common situation. Supposing the server that is supposed to return the child page does not respond for a certain amount of time or returns with non-2xx response, or the connection times out... There can be a plenty of reasons why child may not establish its side of connection in expected time or at all.

Would you expect the same for all method calls?

I don't seem to get the question. If the promise gets rejected then no methods may possibly get called. Can you be a bit more specific in this part?

[Aaronius]

Thanks.

Sorry, on the last question I meant: If the connection is successfully established (the handshake succeeds) and the parent then tries to call a method that the child has exposed (or vice-versa), should the promise returned from the method call get rejected after a certain amount of time (potentially configurable)?

[jrencz]

should the promise returned from the method call get rejected after a certain amount of time (potentially configurable)?

I see it this way: One of possible use cases for Penpal I see is being a replacement for workers (which are not always available as of today) to offload the main thread with non-visible iframe. In that case default timeout may break existing implementations since the very reason one would create a worker is to perform long tasks.

If on the other hand child no longer maintains its side of the connection (e.g. iframe crashed or navigation happened in the iframe: anything that may cause the connection to no longer be considered established but child did not call connection.destroy) the pending promises should be rejected (optimally: with a certain Error subclass or Error with certain message so it can be distinguished) as soon as child absence is detected.

Anyway: I'd see that (configurable method call timeout disabled by default) a welcome enhancement but it's a different topic than establishing connection. Let's maybe discuss that in a distinct issue?

[Aaronius]

Alright, I've published this on the next dist-tag as 3.0.0-alpha.3. You can try it out via npm install penpal@next or npm install penpal@3.0.0-alpha.3.

Give it a shot and let me know how it works for you. You can find the timeout option in the readme here: https://github.com/Aaronius/penpal/tree/v3

[jrencz]

I checked at 3.0.0-alpha.5 and the timeout on parent side looks like it's broken.

Regardless of whether the connection is established or not the iframe is removed after timeout is done.

The problem seems to be in here:

const promise = new Penpal.Promise((resolve, reject) => {
  if (timeout !== undefined) {
    setTimeout(() => {
      reject(new Error('Connection to child timed out after ' + timeout + 'ms'));
      destroy();
    }, timeout);
  }

reject called after resolve (which is the case if connection was established and handleMessage was called) is a noop. But destroy isn't. Let's add clearTimeout somewhere

Another thing I'd suggest is that timeout might be expressed as a promise as well.

how about this:

const promise = new Penpal.Promise((resolve, reject) => {
  if (timeout !== undefined) {
    if (typeof timeout === 'number') { // very naive. Maybe let's check if it's not thenable instead
      timeout = new Promise((res => {
         setTimeout(res, timeout);
      }))
    }
    timeout.then(() => {
      reject(new Error('Connection to child timed out after ' + timeout + 'ms'));
      destroy();
    })
  }

[Aaronius]

Good catch. I'll fix it and add a test. Sorry, I haven't tried the changes in an actual project yet or I would've caught this, but I will soon. Thanks!

[Aaronius]

The issue you mentioned has been fixed in penpal@3.0.0-alpha.6.

[jrencz]

I tested penpal@3.0.0-alpha.6 and the timeout works fine.

When timeout was added as a reason of promise rejection there are 2 errors: timeout and connection destroyed.

How about baking in a way to detect which is which? I suggest there could be TimeoutError and ConnectionDestroyedError thrown instead of generic ones. Then user might:

Penpal
  .connectToChild()
  .promise
  .catch(error => {
    if (error instanceof Penpal.TimeoutError) {
      // do something which will be suitable on timeout
    } else if (error instanceof Penpal.ConnectionDestroyedError) {
      // do something which will be suitable in case child closed the connection
    } else {
      // despair :)
    }
  })

To prevent error constructors from being exposed (which may not necessarily be desired) there could be helpers:

Penpal
  .connectToChild()
  .promise
  .catch(error => {
    if (Penpal.isTimeoutError(error)) {
      // do something which will be suitable on timeout
    } else if (Penpal.isConnectionDestroyedError(error)) {
      // do something which will be suitable in case child closed the connection
    } else {
      // despair :)
    }
  })

This way user won't have to compare error messages. Having helpers will also allow changing implementation of what those errors are so it seems to be even better way than exposing constructors

[Aaronius]

Good call. I'll probably either set the name property on the error object or add a code property like Node does and then expose constants.

[Aaronius]

Error codes added in in 3.0.0-alpha.7. See https://github.com/Aaronius/penpal/blob/05a7c15a9232718a5b7c0d045e9564a18fcc17b2/README.md#errors for details. Thanks for trying out these changes and giving feedback.

[Aaronius]

Actually, hold on...making some tweaks.

[Aaronius]

Okay, you can try it with 3.0.0-alpha.8. See https://github.com/Aaronius/penpal/blob/619741eacf9ac996187b0c008c9e240b9118d80b/README.md#errors for details.

[jrencz]

I think it would be good to expose those constants as named exports es well

[Aaronius]

I'm curious to know where you would draw the line with named exports. For example, early on I toyed with making connectToChild and connectToParent named exports (instead of properties on the default export object) but decided against it. I also thought about changing Penpal.promise into a named export called setPromise and Penpal.debug into setDebug, but, again, decided against it.

[jrencz]

If I was to decide I'd say that things that make sense stand-alone (like error codes but not like promise setter or debug setter which affect the Penpal singleton) may be exported with named exports. This would allow bundlers to reach those values and discard all the rest (if they are capable of such optimisation) in case default export (Penpal singleton) is not imported in the module in question.

Let's imagine that someone wants to create a common handler for things that can go wrong with Penpal connection and this handler should be used in many projects (Real life: I have 10 micro-apps I need to embed within main app. Each uses Penpal and main uses Penpal. I want to handle connection errors the same way: send exception event to GA).

Without paying additional attention it would be very hard to make the handler UMD bundle small without hard-coding error codes. With named export it's at least possible. AFAIK rollup is capable of such transformation. Properly set up webpack should be capable of doing the same (and I'm not thinking about making Penpal an external dependency of such handler package, which would make sense anyway)

[Aaronius]

That makes sense. Thanks.

[Aaronius]

Published in 3.0.0.