feat!: add `<AblyProvider>` and `useAbly` #57

owenpearson · 2023-07-26T10:05:41Z

Resolves #47 #49

adds an ably context provider which makes an ably client available via a useAbly hook. the hook is used by useChannel and usePresence instead of the existing configureAbly and provideAbly functions. in order to properly support multiple clients (and potentially nested s), an optional string id prop is available to set ids which can be passed to useChannel, usePresence, and useAbly to specify a particular client/context.

SDK-3748

danielrhodes · 2023-07-27T22:22:13Z

@owenpearson This is looking like a much improved version of the library. It will work a lot better with React Native, especially during development with fast refreshing.

One big issue I ran into with my app is that when I first load things up, I don't have an Ably auth token so I can't instantiate the Ably client right away. The Ably JS client has worked best for me when it can decide itself when it wants to fetch a new token.

In the current version of the library, not having a client yet creates a race condition and it throws an exception. But in this version, it also throws an exception in useAbly, which is not good practice in React as there is no appropriate way to catch this in JSX outside of an error boundary. I am skeptical many React developers commonly use error boundaries, so an exception here would likely create a WSOD. This also makes a big assumption about how somebody's app is architected, specifically that the screen where they are using Ably can only be shown if the client is ready to go.

The best practice is to handle the case where there is no client yet. In the code I shared in my Gist https://gist.github.com/danielrhodes/7271ad41eb3d189e62f58309a6b5961c, I solve for this in useChannel and usePresence without throwing an error.

owenpearson · 2023-08-03T15:28:23Z

Hey @danielrhodes, thanks for the feedback!

I've been considering this a bit and I don't know if letting our hooks work without an ably client is a good idea. This would result in users of the library having to handle cases where the client does not exist yet, for example, if you useChannel you would now always need to check if the channel even exists because it might not. I'm especially cautious about taking this approach since the useApolloClient hook also throws an error if the client doesn't exist.

If you need to get an Ably token before instantiating your client I think the best way for this to work is by simply not creating the AblyProvider until you're ready to create the client. For example:

if (client) {
  return <AblyProvider client={client}>{children}</AblyProvider>;
} else {
  return <Loading />
}

danielrhodes · 2023-08-03T18:25:56Z

@owenpearson Apollo gives a warning (which is great and it should be done here), but they do not throw an exception. Throwing an exception inside a hook is very problematic because everything is state and declarative. Instead they would return an error object, which is what Apollo and hooks-based HTTP libraries do.

In the example you provided, I still wouldn't be able to use useChannel or usePresence because they expect the provider to be in place. In other words, if they don't find an Ably client in context, it throws. But since useChannel or usePresence are used in a much deeper part of the app and you can't render a hook conditionally, a developer would have to eat the risk that the client isn't ready yet. On the other hand, if the channel does not exist yet due to there being no client, it can be easily handled. Throwing an exception means the hook is essentially rendering conditionally.

For Apollo, you can still use useSubscription (equivalent in this case to useChannel), but it's not going to work if there is no active client. I believe in that case the error property would be not null and the data property would be null.

const { data, error } = useSubscription(...)

useEffect(() => {
  // handle error
}, [error]);

useEffect(() => {
  // handle change in data
}, [data]);

So using that as inspiration, you could do this:

const { error, channel } = useChannel('foo');
const { error } = usePresence('foo');

If you really wanted to make this more hooks friendly, you might even go further:

const { publish, error, subscribe, data } = useChannel('foo');
const { results, hasNext, fetchNext  } = useChannelHistory('foo');

useEffect(() => {
  // handle change in data
}, [data]);

// or

const { publish, error, subscribe } = useChannel('foo', {
  onData: (data) => {
     ...
  }
});

where data would just be whatever was last received.

But that's for another time :-)

owenpearson · 2023-08-04T15:01:35Z

Hey @danielrhodes, thanks again for the feedback. Based on my reading of the invariant docs that line you linked does indeed throw an error. I actually just tested this myself to make sure I wasn't missing anything and sure enough useApolloClient throws a runtime error when used outside of an ApolloProvider (same goes for hooks like useSubscription which depend on useApolloClient).

I do think for the most part we're on the same page here - I absolutely agree that errors which occur after the client has been instantiated (eg. network errors) should not by thrown, but rather conditionally returned by the hook. However, I do believe that the case where a user calls a hook outside of an AblyProvider is a special case where the hook is missing a critical dependency and that should be made as visible as possible by throwing a runtime error.

We're actually planning on improving how errors are handled in the library as part of #55, and our current plan is to expose errors with pretty much the same API as your code snippets 🙂

My main question here is what is preventing you from being able to just mount the AblyProvider and any components which require Ably hooks after the client has been created? I can't think of any reason this wouldn't be possible and AFAICT apollo-client requires this too.

ttypic

It looks really nice! Only concern is mutating the options argument. And also I am still trying to figure out do we really need to support multiple Ably clients in the same app? Because in that case we never clear the context, and it seems like memory leak for me.

ttypic · 2023-08-04T17:28:13Z

src/AblyProvider.tsx

+            } as Types.ClientOptions;
+        }
+
+        (options as any).agents = { 'react-hooks': version };


It is generally advised to avoid modifying incoming function arguments, as this practice can lead to unintended side effects

good point - tbh it was already like this before so i wasn't planning on updating it here but i've added 8b1d000 to fix this

ttypic · 2023-08-06T21:34:59Z

src/AblyProvider.tsx

+// indexed by id, which is used by useAbly to find the correct context when an
+// id is provided.
+type ContextMap = Record<string, AblyContextType>;
+export const contextKey = '__ABLY_CONTEXT__';


I think Symbol may fit better here

i did consider this before but i figured it wasn't really worth it since it would break ie11 compatibility and the risk of a naming collision is basically zero anyway. i've now updated it to use Symbol.for guarded behind a typeof check

ttypic · 2023-08-06T21:40:02Z

src/AblyProvider.tsx

+    const realtime: Realtime =
+        client || new Realtime(options as Ably.Types.ClientOptions);
+
+    let ctxMap: ContextMap = (React.createContext as any)[contextKey];


I would suggest take a look at how react-redux dealing with this https://github.com/reduxjs/react-redux/blob/master/src/components/Context.ts. I believe it's more elegant solution. I don't very much like this one, because we are making assumptions about code we don't control. If at some point React.createContext become frozen solution will stop working.

using globalThis is still making assumptions about code we don't control, but i agree that is a more elegant solution 🙂 i've updated this branch to use similar

ttypic · 2023-08-06T22:15:52Z

src/hooks/useChannel.ts

-        typeof channelNameOrNameAndOptions === 'string'
-            ? assertConfiguration()
-            : channelNameOrNameAndOptions.realtime || assertConfiguration();
+    let id = 'default';


It looks a little fragile, that we need to repeat this lines of codes in every hook. Is it necessary to support multiple ably client instances in the app? Shouldn't we encourage users to use only one?

i would not advise anyone to use multiple clients in the same app but people do ask for it and it's not too hard for us to support so i think it's worth it. i've slightly simplified this code now so hopefully it looks less fragile

ttypic · 2023-08-07T08:02:47Z

src/AblyProvider.tsx

+import { Types } from 'ably';
+import React from 'react';
+
+const version = '2.1.1';


Can we get this right from package.json?

i did actually try this - the reason we don't do this already is that tsc complains about importing modules from outside of compilerOptions.rootDir. obviously it would be nice to have a single source of truth but i figure it's outside of the scope of this PR

adds an ably context provider which makes an ably client available via a useAbly hook. the hook is used by useChannel and usePresence instead of the existing `configureAbly` and `provideAbly` functions. in order to properly support multiple clients (and potentially nested <AblyProvider>s), an optional string id prop is available to set ids which can be passed to useChannel, usePresence, and useAbly to specify a particular client/context.

jamienewcomb · 2023-08-08T10:08:38Z

@stmoreau @owenpearson Can we link the Jira issue to this please?
@stmoreau did we not have a CI run that flags this

We have asked many times for this to happen now :(

owenpearson requested a review from stmoreau July 26, 2023 10:05

owenpearson force-pushed the context branch from 9d43335 to 0642419 Compare July 26, 2023 10:28

stmoreau requested a review from ttypic August 4, 2023 15:55

ttypic suggested changes Aug 7, 2023

View reviewed changes

owenpearson force-pushed the context branch 2 times, most recently from d0c89c8 to 8b1d000 Compare August 7, 2023 14:41

owenpearson added 3 commits August 7, 2023 15:52

docs: update README with context provider API

06cf0ca

fix: avoid mutation of user client options

a3cb538

owenpearson force-pushed the context branch from 8b1d000 to a3cb538 Compare August 7, 2023 14:52

ttypic approved these changes Aug 7, 2023

View reviewed changes

stmoreau approved these changes Aug 7, 2023

View reviewed changes

owenpearson merged commit 9b05bea into integration/v3 Aug 7, 2023

owenpearson mentioned this pull request Aug 9, 2023

usePresence throws error if Ably is not configured #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add `<AblyProvider>` and `useAbly` #57

feat!: add `<AblyProvider>` and `useAbly` #57

owenpearson commented Jul 26, 2023 •

edited by jira bot

Loading

danielrhodes commented Jul 27, 2023 •

edited

Loading

owenpearson commented Aug 3, 2023

danielrhodes commented Aug 3, 2023 •

edited

Loading

owenpearson commented Aug 4, 2023

ttypic left a comment

ttypic Aug 4, 2023

owenpearson Aug 7, 2023

ttypic Aug 6, 2023

owenpearson Aug 7, 2023

ttypic Aug 6, 2023

owenpearson Aug 7, 2023

ttypic Aug 6, 2023

owenpearson Aug 7, 2023

ttypic Aug 7, 2023

owenpearson Aug 7, 2023

jamienewcomb commented Aug 8, 2023

feat!: add <AblyProvider> and useAbly #57

feat!: add <AblyProvider> and useAbly #57

Conversation

owenpearson commented Jul 26, 2023 • edited by jira bot Loading

danielrhodes commented Jul 27, 2023 • edited Loading

owenpearson commented Aug 3, 2023

danielrhodes commented Aug 3, 2023 • edited Loading

owenpearson commented Aug 4, 2023

ttypic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienewcomb commented Aug 8, 2023

feat!: add `<AblyProvider>` and `useAbly` #57

feat!: add `<AblyProvider>` and `useAbly` #57

owenpearson commented Jul 26, 2023 •

edited by jira bot

Loading

danielrhodes commented Jul 27, 2023 •

edited

Loading

danielrhodes commented Aug 3, 2023 •

edited

Loading