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

On connect callback #17

Closed
tleish opened this issue Jul 10, 2022 · 12 comments
Closed

On connect callback #17

tleish opened this issue Jul 10, 2022 · 12 comments

Comments

@tleish
Copy link

tleish commented Jul 10, 2022

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

tleish commented Jul 11, 2022

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

  • initial connection successful:
    {reconnect: false, restored: false}
  • initial connection failed, get new token, successful connection
    {reconnect: false, restored: false}
  • connection lost (offline), get new token (online), successful connection
    {reconnect: false, restored: false}

Double Subscribe

When using the above logic, the following occurs

  • initial connection successful:
    subscribe sent twice
  • initial connection failed, get new token, successful connection
    subscribe sent once
  • connection lost (offline), get new token (online), successful connection
    subscribe sent twice

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

palkan commented Jul 11, 2022

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

palkan commented Jul 12, 2022

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.

@palkan palkan closed this as completed Jul 12, 2022
@tleish
Copy link
Author

tleish commented Jul 12, 2022

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

palkan commented Jul 13, 2022

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

@tleish
Copy link
Author

tleish commented Jul 13, 2022

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

tleish commented Jul 13, 2022

image

@palkan
Copy link
Member

palkan commented Jul 13, 2022

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

tleish commented Jul 13, 2022

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 added a commit that referenced this issue Jul 13, 2022
This prevents from closing a channel and rejecting a subscription promise in case we manually connect/disconnect cable (or when using a token refresher)

Ref #17
@palkan
Copy link
Member

palkan commented Jul 13, 2022

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

tleish commented Jul 13, 2022

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

@palkan
Copy link
Member

palkan commented Jul 13, 2022

Just released 0.4.1.

Thanks for you help!

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

No branches or pull requests

2 participants