[Frame] Refactor to use createContext API

[danrosenthal]

Addresses #785.

[danrosenthal]

I'm noticing an issue where TypeScript yells at me for polaris and context being missing from ContextualSaveBar props in tests. Possibly related, I have to pull polaris and context out of this.props in ContextualSaveBar lifecycle methods. Anyone understand what is going on here?

[danrosenthal]

Also feeling a little weird about putting everything onto a context object in props. @AndrewMusgrave can you explain the thinking behind this a bit? Does this feel right to you to keep as a pattern?

[dleroux]

I believe the Toast tests will need to be updated with the Provider just like you did for the ContextualSavebar. Same with Loading. Looks like similar tests in ContextualSaveBar are passing because they've been updating with the right context.

[danrosenthal]

Looks like the typing issue was to do with using the wrong version of TS in VSCode. So that problem is resolved. @dleroux this is still a WIP, so I haven't updated the other tests yet. Those tests failing was expected.

[AndrewMusgrave]

Looks like the typing issue was to do with using the wrong version of TS in VSCode. So that problem is resolved. @dleroux this is still a WIP, so I haven't updated the other tests yet. Those tests failing was expected.

Seems there is an issue with compose returns the type with context in TS v.3.2.2. 🔥 C should fix it.

Also feeling a little weird about putting everything onto a context object in props. @AndrewMusgrave can you explain the thinking behind this a bit? Does this feel right to you to keep as a pattern?

I think this will be ok to keep as a pattern. Rather than directly using a render prop this'll allow us to keep our render logic cleaner and force similar implementations across context usage. If we decide to change something in the future we'll only have to change it in the HoC. And this'll allow us to use the same injector pattern we're familiar with (e.g. withAppProvider, withSticky).

I decided to call the prop context to keep it consistent with old react context (this.context). Although this can definitely be changed.

It's a very common pattern to pass context down as props, it's just not normally called context. You'll see it in most major libraries, for example redux, react-router, apollo, etc.

[danrosenthal]

That rationale makes sense to me @AndrewMusgrave. Would you see us moving polaris context under the context object once we refactor AppProvider to use the new context API?

[AndrewMusgrave]

I left a few comments, and we'll probably want to squash the commits but it's looking good!

Would you see us moving polaris context under the context object once we refactor AppProvider to use the new context API?

It definitely could live there but it seems we currently use it for more app level utilities and configuration. We originally moved properties inside polaris, like easdk for example to namespace it. So we would be losing that.

[AndrewMusgrave]

Just curious, why do we need to setProps on frame now?

[danrosenthal]

This is an alternative to mounting and unmounting the child component, since we no longer have direct access to the child component. Calling setProps on Frame with children that have new props triggers the componentDidUpdate lifecycle method for those children.

[BPScott]

Seems there is an issue with compose returns the type with context in TS v.3.2.2. 🔥 C should fix it.

Would you be able to fire up a PR for this? Telling people to stick with TS 3.1.x is a bit rubbish

[lemonmade]

I took a little stab at improving the typing of decorators, I think having the following types should allow you to grab just the static types on the decorated component:

type StaticFields<Object> = {
  [Key in keyof Object]: Key extends 'prototype' ? never : Key
}[keyof Object];
type Statics<Object> = Pick<Object, StaticFields<Object>>;

// Example
class MyComponent extends React.Component {
  static foo = 'bar';
  static Baz(qux: string) {
    return <div>{qux}</div>;
  }

  render() {
    return null;
  }
}

type StaticMembers = Statics<typeof MyComponent>; // {foo: string; Baz: () => JSX.Element}

[danrosenthal]

@dleroux, @AndrewMusgrave, @tmlayton this is ready for another look

[AndrewMusgrave]

LGTM 👍

[AndrewMusgrave]

We use the {id: string} interface a lot. Maybe it would be worth it to bring this out to the main type file and name it ID?

[danrosenthal]

@lemonmade is the Apollo issue preventing Provider/Consumer components at the Frame level no longer blocking this PR merging?

[lemonmade]

Yes, we've shipped and reverted the Apollo upgrade several times so we are getting close, but it is currently blocking.

[tmlayton]

Want to create a 4.0 branch and target it with this PR?