anycable / anycable-client

AnyCable / Action Cable JavaScript client for web, Node.js & React Native
MIT License
92 stars 15 forks source link

On connect callback #17

Closed tleish closed 2 years ago

tleish commented 2 years ago

I have a scenario where a URL can return a 304 with the prior cached page including an AnyCable JWT token. In the event of a cached page, when the browser renders the cached page with an expired JWT token, it cannot connect.

From the examples I created the following to update the token:

import { createCable } from '@anycable/web';

export default createCable({
  tokenRefresher: async (transport) => {
    let response = await fetch('/cable_url.json');
    let data = await response.json();

    // Update URL for the underlying transport
    transport.setURL(data['url']);
  },
});

This works at updating the token when the prior cached token expired. However, when combined with <turbo-cable-stream-source /> the channels are not initiated since the attempt to establish the channels occurred prior when the elements were first added to the DOM. As a workaround, I added the following to remove and re-add the elements.

export default createCable({
  tokenRefresher: async (transport) => {
    //...

+   // Remove and re-add turbo-cable-stream-source elements to reconnect to channel
+   document.querySelectorAll('turbo-cable-stream-source').forEach((element) => {
+     element.parentNode.replaceChild(element, element);
+   });
  },
});

When testing the above code, the elements try to reconnect before the new connection is established and I get an error:

index.js:257 Uncaught (in promise) NoConnectionError: No connection
    at Cable._subscribe (index.js:257:46)
    at Cable.subscribe (index.js:245:31)
    at Cable.subscribeTo (index.js:466:17)
    at TurboStreamSourceElement.connectedCallback (stream_source_element.js:12:54)
    at cable.js:17:28
    at NodeList.forEach (<anonymous>)
    at tokenRefresher (cable.js:16:62)
    at async index.js:117:7
    at async index.js:136:11

Wrapping it in a setTimeout resolves the issue, but feels a hacky.

export default createCable({
  tokenRefresher: async (transport) => {
    //...

    // Remove and re-add turbo-cable-stream-source elements to reconnect to channel
+   setTimeout(() => {
      document.querySelectorAll('turbo-cable-stream-source').forEach((element) => {
        element.parentNode.replaceChild(element, element);
      });
+   }, 1);
  },
});

Is there a better approach, for example callback available to initiate this when the connection is established?

tleish commented 2 years ago

I've created the following, which feels a little better but has one flaw with it.

import { createCable } from '@anycable/web';

// By default, the current page is loaded in the background,
// and the action-cable-url (or cable-url) meta tag is used to update
// the connection url
const cable = createCable({
  tokenRefresher: async (transport) => {
    let response = await fetch('/cable_url.json');
    let data = await response.json();

    // Update URL for the underlying transport
    await transport.setURL(data['url']);
  },
});

cable.on('connect', (_event) => {
  document.querySelectorAll('turbo-cable-stream-source').forEach(async (element) => {
    await element.connectedCallback();
  });
});

export default cable;

Connect Event

It seems odd to me that the connect event object is always the same

Double Subscribe

When using the above logic, the following occurs

Because of this, the custom code feels inefficient to send subscribe twice, especially on initial connection successful.

Custom reconnect/restored

The custom code can avoid the double subscribe events when creating custom logic for reconnect/restored status.

+ let connection = { reconnect: false, restored: false };
+ cable.on('close', (_event) => {
+   connection.reconnect = true;
+ });

cable.on('connect', (_event) => {
+ if (connection.reconnect && !connection.restored) {
    document.querySelectorAll('turbo-cable-stream-source').forEach(async (element) => {
      await element.connectedCallback();
    });
+ }
+ connection.reconnect = false; // reset reconnect variable
+ connection.restored = true; // the next time connect is called, it will be a restored event
});

Still not sure if this is the best approach for this.

BUG: Uncaught (in promise) DisconnectedError: Disconnected

Note that in the scenario initial connection failed, get new token, successful connection the browser console logs the following error (twice).

Uncaught (in promise) DisconnectedError: Disconnected
    at ActionCableProtocol.receive (index.js:77:17)
    at Cable.handleIncoming (index.js:199:35)
    at index.js? [sm]:4:46
    at Array.forEach (<anonymous>)
    at Object.emit (index.js? [sm]:4:33)
    at ws.onmessage (index.js:97:20)

The "DisconnectedError: Disconnected" is valid, but the console logging an error with "Uncaught (in promise)" seems like a bug to me in anycable-client. This does not occur with the scenario connection lost (offline), get new token (online), successful connection.

palkan commented 2 years ago

Thanks for the report!

I think, we should make cable.subscribe(channel) kind of pessimistic: wait for subscription rejection/confirmation independently of the cable state.

I've started working on this, but that's turned out a bit more trickier than I expected; hope to release a new version later this week.

palkan commented 2 years ago

Released 0.4.0 (core, web, other packages use custom versioning but also updated to match the core dependency).

The most important change is that cable.subscribe(channel) automatically handles network issues and keeps the channel subscribed until channel.disconnect() or cable.disconnect() (or server sent disconnect without reconnect) is called.

Please, let me know if there are still some issues.

tleish commented 2 years ago

Just tried it out. I still get the "Uncaught (in promise) DisconnectedError: Disconnected" error in the dev console, but otherwise working, but it appears to be working without the additional logic of refreshing turbo-stream elements.

Thanks for the quick fix.

palkan commented 2 years ago

Do you see this error when calling cable.connect()?

tleish commented 2 years ago

I see this error when the page initially loads with a cached/expired token and the javascript renews the token. The code does renew the token, but the library also logs this error in the javascript console.

// cable.js
import { createCable, fetchTokenFromHTML } from '@anycable/web'

// By default, the current page is loaded in the background,
// and the action-cable-url (or cable-url) meta tag is used to update
// the connection url
export default createCable({tokenRefresher: fetchTokenFromHTML()})

promise-error

tleish commented 2 years ago

image

palkan commented 2 years ago

Thanks for the reproduction!

Yeah, that's where the error is initiated. The question is which promise is rejected and not handled. (I'm pretty sure it's await subscribeTo(...), but not yet sure why).

tleish commented 2 years ago

The following change removes the error for me, but not sure if it's the right change.

export class Cable {
  //...
  subscribeTo(ChannelClass, params) {
      //...
-    return this.subscribe(channel).then(() => channel)
+    return this.subscribe(channel).then(() => channel).catch(() => channel)
  }
}
palkan commented 2 years ago

I pushed a fixed to master (see the change log for the explanation).

I plan to push one more minor fix (discovered a new edge case for the token refresher), and then release a new patch version of the core package.

tleish commented 2 years ago

I tested the master branch and can confirm that I no longer see the console error. Looking forward to the new patch version.

palkan commented 2 years ago

Just released 0.4.1.

Thanks for you help!