UIAPPS-104 Create a UI Extensions react package

[joneshf-dd]

Motivation

• Jira Issue: https://datadoghq.atlassian.net/browse/UIAPPS-104
• We want to make it easier to use UI Extensions with react.

Changes

There's a lot here, but it's mostly infrastructure changes.

• We make it so any package that starts with @datadog/ui-extensions- gets locked together with the same version. This took some extra work to make changesets still work the way we want. Hopefully if Add support for fixed packages changesets/changesets#690 merges, it'll mean we can remove a lot of this behavior.
  There's also a patch that we had to do to the @changesets/assemble-release-plan package that we should ask about.
• We make a react package that provides a hook for using the context. The context is one of those things that can solve a lot of problems, but using it is not that straight-forward. Hopefully by providing a hook that does all the hard work, we can push more people toward using it.
  Also, we should be able to build upon it to make other aspects of UI Extensions easier as well.
• Finally, we actually use the new package in the examples. This dogfooding should help guide the direction of this new package, while also showing its usefulness.

Testing

• Mostly, take a look at the examples and see if they make sense.

Releases

Choose one:

• No release is necessary.
  If you're only updating examples/documentation, this is likely what you want.
• All packages that need a release have a changeset.

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @datadog/[email protected]
npm install @datadog/[email protected]
npm install @datadog/[email protected]
npm install @datadog/[email protected]
# or 
yarn add @datadog/[email protected]
yarn add @datadog/[email protected]
yarn add @datadog/[email protected]
yarn add @datadog/[email protected]

[joneshf-dd]

❓ It kind of feels like we should take this opportunity to show some decent patterns for dealing with the other two states: initializing, and handshake failure. I didn't want to make too big of a change in case that wasn't wanted. What do y'all think?

[nmuldavin]

The funny thing is that when the frame is in loading or error state, the iframe is invisible so it almost doesn't matter what the app does, short of blowing up. The right pattern is honestly probably just

if result.type === 'initialized' {
  return null
}

// skip all other logic

[joneshf-dd]

Oh right! Makes me wish we didn't have to do something about those other states and could just return the context only.

Maybe what would be more useful for actually rendering a component is not the hook for context, but a component that passes down the context once it's ready and only renders once it has a context. So you'd do something like:

import { WithContext } from '@datadog/ui-extensions-react';
import { Context, init } from '@datadog/ui-extensions-sdk';

type WidgetProps = {
    context: Context;
};

function Widget(props: WidgetProps) {
    // Work with the actual `Context` in `props.context`.
    …
}

export default function render() {
    ReactDOM.render(
        <React.StrictMode>
            <WithContext client={client}>
                <Widget />
            </WithContext>
        </React.StrictMode>,
        document.getElementById('root')
    );
}

Or something, general idea being that the hook isn't as helpful as it could be since it's still too low-level.

[joneshf-dd]

Or maybe, we just don't deal with the error at all and the return type of useContext becomes Context | undefined?

[joneshf-dd]

These are peer dependencies because we depend on their APIs for this package's own APIs.

[joneshf-dd]

This was mostly lifted from the SDK config. Does the global (DD_REACT) make sense here?

[joneshf-dd]

Just noting that this is explained more fully in the patches/README.md file.

[nmuldavin]

🔊

[nmuldavin]

cool!

[nmuldavin]

I have an alternative schema proposal: Many react developers are familiar with hook patterns for async operations that look like what you end up using from useQuery:

const { data, isLoading, error } = `useQuery(...)`;

Rather than using a discriminated union, we could try to mimix that pattern:

interface useContextResult {
  isLoading: boolean;
  context: uiExtensionsSDK.Context | null;
  error: Error | null
}

const { isLoading, error, context } = useContext(client);

What do you think? Of course it's the same information in the end, just attempting to follow existing patterns more closely

[joneshf-dd]

tl;dr; I'm okay with whatever here. I'll update this next week unless the longer response compels you otherwise. You don't have to read the rest of this, but can if you'd like.

---

I have a more thoughts about this, but this response ended up being long enough as it is. If you'd like me to go into even more detail lemme know.

Whatever the API of this ends up being, there's maybe a more important point to make. I don't see this particular hook being that useful for App developers in their day-to-day work, but very useful for IDX. There's at least two reasons why I say this:

1. The SDK Context mostly has optional data, and trying to drill down on that seems harder than it needs to be.
2. I think most App developers would rather use something else aside from Context.

The second point I think is where we're going to get the most benefit out of having a package like this. I was playing around with a useCustomWidgetOptions hook that built on top of this useContext hook. It seemed to remove the majority of the explicit references to the Context. I feel like it'd be similar for most other hooks we'd build: useTimeframe, useColorTheme, etc. All of these have decent defaults and can plaster over the different states of useContext no matter what its API looks like.

Context gives a lot of data, and it can be useful to solve a lot of problems we run into. For sure we should make it available to App developers can unblock themselves if we don't provide an easy thing to use. But, it feels more like a low-level primitive than something we expect people to work with in day-to-day. And we should try to capture those patterns that people do instead. I didn't want to add that to the documentation because there's nothing else to point people to at the moment.

To that end, I feel like we should optimize correctness over familiarity. In this situation, familiarity makes it harder to know you're doing the right thing when using useContext. There's eight different states to deal with:

isLoading	context	error
false	null	null
false	null	Error
false	Context	null
false	Context	Error
true	null	null
true	null	Error
true	Context	null
true	Context	Error

Whereas with the discriminated union there's only three actual states:

isLoading	context	error	Actual states
false	null	null	"initializing"
false	null	Error
false	Context	null
false	Context	Error
true	null	null
true	null	Error	"handshake failure"
true	Context	null	"initialized"
true	Context	Error

For something that seems like it'll end up being more internal than external, it seems like we should optimize for simplifying on our end.

Of course, if my read on useContext and the SDK Context in general is off, then yeah we should optimize for App developer familiarity.

[nmuldavin]

A summary of in-person discussions:

1. I like the pattern of this hook returning Context | null to start with
2. If we need extra hooks for handling handshake errors on the developer's end, we can do so later and explicitly
3. I like that we have a data model with shape, it makes stuff discoverable. The data model will come to be more and more complex over time. Ideally there is a way to both have a general and explorable data model and specificity for common usecases in a way that feels layered rather than disjoint. One idea discussed was a context selector pattern: useContextSelector(getDashboardTimeframe).
4. I like the idea of automatically calling init inside this hook or another wrapper. The one usecase it won't cover is defining an auth provider, but this is generally done outside react at present. Later we may move to a setProvider() model anyways

[joneshf-dd]

I tried to do the fourth thing on the list, calling init within the hook. But I can't get the tests to pass in that case. Leads me to one of two ideas: either we need to mock things differently (most likely), or this might be a bug in jest that's fixed in a newer version (less likely).

In either case, I'm going to punt on it until we can add some tests for that behavior that pass.

[nmuldavin]

Good catch, I probably would have forgotten to do this

[joneshf-dd]

Credit goes to the testing package. It pointed out so many failures and misunderstandings while trying to build this hook. It's way more complex than I'd thought!

[nmuldavin]

Maybe the react api patterns being debated here can be an engineering sync topic?

[nmuldavin]

I think this is ready to merge, press the button! I'm really excited about this getting out, i will use it!

We should poll the team about whether they would prefer a useContext hook or a component wrapper, but my take is that having both is a good thing depending on user preference, so this work is valuable anyways

[joneshf-dd]

Thanks for the multiple reviews and helping figure out things!