Add DebugEvent system to urql client for devtools and debugging

[andyrichardson]

About

There are a lot of moving parts going on in Urql exchanges, these include:

• Operation specific events (i.e. “operation deduped”)
• Exchange state events (i.e. “cache state has been changed”)

Currently we provide a means for communicating some operation specific events but not all. There are no means to communicate exchange specific events which are not coupled to an operation.

This proposal is the addition of an event target. It's a pub/sub mechanism for exchanges to publish the aforementioned events and for devtools, a user, or any other third party to consume.

Usage

Listening to events in the console:

const client = createClient({ ... });
client.debugTarget.addEventListener(console.log);

Passing the client to a third party library (such as devtools);

myDevTool(client);

Types

Types only apply if a known key is used. This prevents conflicts with core while also allowing for ambiguous types.

If third parties want to add a type, can use declaration merging

import { DebugEventTypes } from 'urql'

declare module "urql" {
  export interface DebugEventTypes {
    newType: { dataProp: string };
  }
}

Todo

• Disable in prod
• Add docs on debugging events
• Add babel plugin to remove debug events in prod bundles (cc @JoviDeCroock)

[changeset-bot]

🦋 Changeset is good to go

Latest commit: dfd7c24

We got this.

This PR includes changesets to release 4 packages

Name	Type
@urql/exchange-graphcache	Patch
@urql/exchange-multipart-fetch	Patch
@urql/exchange-retry	Patch
@urql/core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[wgolledge]

This looks awesome!

I'm guessing this could, down the line, remove the need for the devtools exchange?

One more point on the event dispatching - I wonder whether it could make sense to have an optional dispatch system applied to every exchange that could be invoked (if the exchange desires) with a message and optional data (and the type if it can't be inferred)? It could save having to pass the client through exchanges 🤷‍♂

[andyrichardson]

Cheers for the thoughts @wgolledge

I'm guessing this could, down the line, remove the need for the devtools exchange?

Yes totally, instead we could just pass the client to a devtools function.

One more point on the event dispatching - I wonder whether it could make sense to have an optional dispatch system applied to every exchange that could be invoked (if the exchange desires) with a message and optional data (and the type if it can't be inferred)? It could save having to pass the client through exchanges 🤷‍♂

Initially I thought it would be a good idea to only pass the dispatch function but then realised that we already pass the client (which contains the EventTarget) so it felt pretty redundant.

I think most exchanges will end up with a function like this:

const myExchange = ({ client, forward }) => {
  const publishMessage = (m) => client.eventTarget.dispatchEvent({ ...m, source: "myExchange" });
  // ...
}

[wgolledge]

Awesome - yep looks good as far as I can see! Will wait for @kitten and/or @JoviDeCroock to weigh in

[sofiapoh]

I like this idea, the changes have a pretty minimal impact too.

[andyrichardson]

Update -

A few example of an event we might see:

{
  type: "fetch",
  message: "A network request has been made",
  operation,
}
{
  type: "refetch",
  message: "Attempting to refetch a failed network request",
  operation,
  data: { 
    retries: 1,
  }
}

Required attributes are now:

• type (UID for event)
• message (human readable description)
• operation (associated operation)
• data (optional additional data)

This also means we need to change our devtools implementation so that we no longer listen via an exchange but instead for events. That way we streamline every message to look similar to the above.

[kitten]

@andyrichardson this seems like the way to go 👍 in what cases do we need to provide additional data? It'd be great if we kept this more on the untyped side, so that adding new debugging events would be easy.

What I basically mean is, if we make this a log call (naming-wise as well as in how it looks to contributors), then adding new calls could be useful in Jest / development as well as being very flexible when people go and add custom logging/debugging events in new exchanges.

So for instance, if someone goes and makes a retryOrAuth exchange, they may add

log('auth', 'Refreshing the current access token since it's expired);

So for them this would look just like console.log. It's also basically the same as what we currently do, we'd just allow for type to be raw strings as well.

Edit: I just saw that you're already taking string event types into account, but the other tl;dr then: can the call be info or log without being specific to a client? And can we make the inputs be multiple arguments instead of a raw object, so it feels more like console.log, our our invariant / warn helpers?

[andyrichardson]

Thanks for the response @kitten!

What I basically mean is, if we make this a log call (naming-wise as well as in how it looks to contributors), then adding new calls could be useful in Jest / development as well as being very flexible when people go and add custom logging/debugging events in new exchanges.

One of the things I want to really make clear with this is that it's an event bus for the purpose of debugging - not for logging events. While making it work like console.log does make things easier to get started, I think it sets a precedent that doesn't really reflect what's going on.

I wouldn't advise people use this for testing as it exposes implementation details. In a browser however, events could very easily be logged to the console.

const client = createClient({ ... });
client.eventTarget.addEventListener(console.log);

Every exchange action is associated with an operation, hiding that information when dispatching the event leaves room for ambiguity (not something you want when debugging).

The implementation of the event target is basically a 1-for-1 copy of EventTarget. If you want to trim things down a little, we could pass dispatchEvent as an argument to exchanges but I don't see too much benefit for that.

It'd be great if we kept this more on the untyped side, so that adding new debugging events would be easy.

Have you tried dispatching a custom event? There's nothing stopping you from doing that - the types are there for type safety and to prevent conflicts between third-party exchanges 👍

[kitten]

So regardless of this being associated with testing or debugging (those were just other use-cases that came to mind), I believe the client.eventTarget.dispatchEvent naming doesn't immediately indicate that this isn't a core part of the package / client's architecture.

So while the terms "info" or "logging" (the latter indeed being inaccurate) may not be what we want here, I think it may make sense to replace the term "event" here, e.g. client.debugEvents.dispatch or client.info.dispatch, etc.

Overall I believe it should be clear to someone using this or reading our exchanges that this isn't something they need to learn, or something that we have to mention explicitly in the "Concepts" docs to avoid confusion.

To us this is just an addition, but I think using the term "events" in this case without other hints in the code to its purpose could make it be confused with streams, since we use the term "events" frequently to explain what streams/observables are. (ref: https://formidable.com/open-source/urql/docs/concepts/stream-patterns/)

[andyrichardson]

Sure thing @kitten I wasn't aware that your issue was with the use of the term eventTarget and I've renamed it to something less generic - debugTarget

To us this is just an addition, but I think using the term "events" in this case without other hints in the code to its purpose could make it be confused with streams, since we use the term "events" frequently to explain what streams/observables are

Events are core to JS (EventTarget, addEventListener, dispatchEvent) and while I agree, some of the naming conflicts are confusing, I think the problem is with our Urql terminology

[kitten]

Ok, I think we're only pending merging #628 into this branch and adding a changeset note for @urql/core.