feat!: add <AblyProvider> and useAbly

[owenpearson]

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]

@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]

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]

@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]

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]

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

[owenpearson]

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]

I think Symbol may fit better here

[owenpearson]

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]

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.

[owenpearson]

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]

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?

[owenpearson]

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]

Can we get this right from package.json?

[owenpearson]

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

[jamienewcomb]

@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 :(