RFC: New version of context #1

acdlite · 2017-12-06T05:38:13Z

Superseded by #2. Please go there instead. I encountered a GitHub bug and had to re-open.

A proposal for a new version of context addressing existing limitations.

TODO:

Implementation notes
High-priority version of context, for animations. (May submit this separately.)

gaearon · 2017-12-06T12:19:43Z

text/0000-new-version-of-context.md

+```js
+type Theme = 'light' | 'dark';
+// Pass a default theme to ensure type correctness
+const ThemeContext: Context<Theme> = new React.createContext('light');


Unnecessary “new”?

What is the purpose of the string argument to createContext? Is that its initial value?

react-broadcast currently uses a channel prop that determines the key name that it uses inside context.broadcasts, but I think this is different.

That's the default value, which just happens to be a string in this example. The default value provides type safety in the case where a consumer is rendered without a provider.

gaearon · 2017-12-06T12:20:40Z

text/0000-new-version-of-context.md

+  render() {
+    return (
+      // Pass the current context value to the Provider's `value` prop.
+      // Changes  are detected using strict comparison


Maybe Object.is? Thinking of NaN.

Good point, that's what shallowEqual uses. I think I'll still refer to it as strict comparison in most places, with an explanation in the design section.

I recently added a compareValues prop to react-broadcast that defaults to making a strict comparison but lets users swap it out if they really need to. Seems like a good spot for an escape hatch.

One of our goals with this API is to encourage immutability, so making it slightly less ergonomic for libraries that rely on mutation is a feature in my view, not a bug. The escape hatch is to create a shadow copy of the container to trigger the change.

gaearon · 2017-12-06T12:21:36Z

text/0000-new-version-of-context.md

+      // Changes  are detected using strict comparison
+      <ThemeContext.Provider value={this.state.theme}>
+        <button
+          onClick={state =>


This should be event?

Seriously, Andrew? "Oh this should be an updater, I'll just add a state argument..."

gaearon · 2017-12-06T12:25:47Z

text/0000-new-version-of-context.md

+}
+```
+
+`createContext` requires a default value to ensure type correctness.


gaearon · 2017-12-06T12:28:58Z

text/0000-new-version-of-context.md

+
+If a consumer is rendered without a matching provider as its ancestor, it
+receives the default value passed to `createContext`, ensuring type safety. We
+will log a warning in development.


Should context objects be named? Otherwise it’s pretty tricky to show a reasonable warning message.

Also, is default value always bad? I could see people wanting to rely on it (without seeing the warning).

Should context objects be named? Otherwise it’s pretty tricky to show a reasonable warning message.

Good idea. We could use a transform similar to the one we have for createClass? I'd rather not have to pass it manually.

Also, is default value always bad? I could see people wanting to rely on it (without seeing the warning).

Yeah, I considered this. I think there are some valid use cases (a themed component that has a default look). But it's usually it's a mistake. I'm thinking about the case where a user renders a Consumer with a Provider, gets the default value, and it takes them a while to realize why the value isn't what they expect.

What if we keep the warning, and add an allowDetached option that suppresses it?

We could use a transform similar to the one we have for createClass?

That one was a bit convoluted IMO. I'd prefer we avoid creating more transforms like this in the future. It's annoying to handle all different ways people might import something, also makes non-JS languages second class. I also don't think in practice you'd create contexts so often that it's hard to name them.

What if we keep the warning, and add an allowDetached option that suppresses it?

Maybe suppressMissingContextWarning on the Consumer?

Maybe suppressMissingContextWarning on the Consumer?

I think that's a great idea. We recently added a <Subscriber quiet> prop to prevent <Subscriber>s from complaining when they're rendered out of context.

Nice! Don't really care what we name it

Regarding the debug name, I would just rather not make it required, especially since it's only for dev. That's how createClass worked and why I suggested using a Babel transform.

gaearon · 2017-12-06T12:32:42Z

text/0000-new-version-of-context.md

+
+We will deprecate the existing API either simultaneous to the new API's
+introduction, or after some period of time to allow users to migrate. We can't
+use a codemod to automatically convert, so this may take a while.


We definitely don’t want to deprecate until big players have converted. We’ll need to chat to popular libraries first and work with them.

This should be a simple naming swap for us. We're already planning on cutting a new release that uses react-broadcast soon. Could possibly be suggested as something to help people to get ready for this change until it's fully baked.

If you update the React Broadcast API to match the proposal (once it's finalized), it could even serve as a drop-in polyfill :D

ryanflorence · 2017-12-06T16:35:58Z

I like it, its nearly identical API to our first version of react-broadcast that was called react-context-emission minus the crazy naming convention

https://unpkg.com/react-context-emission@1.0.1/README.md

const { GeoEmitter, GeoSubscriber } = createContextEmission('geo')

A proposal for a new version of context addressing existing limitations.

acdlite · 2017-12-06T20:56:22Z

@ryanflorence Pretty cool! Just so nobody reading this is confused, one difference is that the argument passed to React.createContext in this proposal is the default value. If a consumer is rendered without a provider, the default value is used. This means context can be statically typed!

type Provider<T> = React.Component<{
  value: T,
  children?: React.Node,
}>;

type Consumer<T> = React.Component<{
  children: (value: T) => React.Node,
}>;

type Context<T> = {
  Provider: Provider<T>,
  Consumer: Consumer<T>,
};

interface React {
  createContext<T>(defaultValue: T): Context<T>;
};

pspeter3

I think this is a drastic improvement over the current context implementation. The one thing I'm concerned about is the render prop API for the Consumer.

pspeter3 · 2017-12-06T20:56:25Z

text/0000-new-version-of-context.md

+const ThemeContext: Context<Theme> = React.createContext('light');
+
+class ThemeToggler extends React.Component {
+  state = {theme: 'red'};


This doesn't match the theme type. Maybe that was intentional?

Oops, oversight. Good catch!

jlongster · 2017-12-06T21:01:59Z

This means context can be statically typed!

Couldn't the type just be Optional<T>? (or whatever your type system's version of Optional is.) What is the use case for allowing consumers to be rendered for detached? I can't think of any, and seems like it should throw a hard error to make sure the user set things up correctly. (I run components in isolation in tests and stuff, but I'm happy to render a top-level component that provides the mock value for my tests in there. Point is, in those cases, you'll likely need something other than the default value if you're doing that kind of thing)

I love the render prop API.

acdlite · 2017-12-06T21:04:58Z

@jlongster See the "Unresolved questions" section:

Should we warn if a consumer is rendered without a matching provider?

There are valid use cases for relying on the default context value. In many
or most cases, it's likely a developer mistake. We could print a warning in
development when a consumer is rendered without a matching provider. To supress
the warning, the developer passes true to allowDetached.

jlongster · 2017-12-06T21:06:14Z

I saw that but it doesn't actually list the valid use cases :) I would argue we should treat it always as a mistake and avoid an additional allowDetached prop.

acdlite · 2017-12-06T21:08:54Z

I think the use cases are similar to defaultProps. Like a button that has a default theme, but changes once you place it inside a Theme provider.

acdlite · 2017-12-06T21:10:58Z

We're also considering the ability to set active={false} to stop listening for changes. I couldn't think of any uses cases except ones that depend on time or animation (which I'll likely submit as a separate proposal), so I left it out for now.

milesj · 2017-12-06T21:12:03Z

text/0000-new-version-of-context.md

+```
+
+To update the context value, the parent re-renders and passes a different value.
+Changes to context are detected using `Object.is` comparison. (Referred to as


May have overlooked it, but how exactly is a change detected in the consumer?

When the provider receives a new value, React re-renders the consumers.

I'll add more details about the implementation shortly.

acdlite · 2017-12-06T21:12:06Z

Weird, I pushed a commit but it's not showing up here...

jaredpalmer · 2017-12-06T21:13:21Z

This is awesome. I like the render prop API as it just seems like less ceremony and typing. I agree with @jlongster about allowDetached too

milesj · 2017-12-06T21:13:44Z

text/0000-new-version-of-context.md

+
+In the following major release, we'll remove the old API.
+
+# Unresolved questions


I think a life-cycle hook in regards to context changes should be discussed. Relying on a child function render for detecting this isn't exactly feasible for all situations. Perhaps another prop on the consumer?

<Consumer onChange={this.handleChange} />

mjackson · 2017-12-06T21:19:12Z

Hmm, looks like my previous comments got buried in the "show outdated" view. Please pardon for re-posting down here...

The ability for subscribers to render out of context is useful primarily in testing scenarios where people are too lazy/don't want to set up the correct context before they run their test. We heard a lot of feedback about this in early versions of react-router with our <Link> element. People wanted to render a component with a <Link> inside it w/out having to set up the rest of the router context. In these situations, using the context's default value is fine. react-broadcast recently introduced a <Subscriber quiet> prop that serves this purpose.
We recently added a compareValues prop to react-broadcast that allows publishers to declare a function they can use to compare values to see if they need an update or not. This seems like a useful escape hatch since we can't assume everyone is using immutable types.

jlongster · 2017-12-06T21:20:41Z

I think the use cases are similar to defaultProps. Like a button that has a default theme, but changes once you place it inside a Theme provider.

The difference to me is props are meant to be used within the component. Context is meant to be used with a Provider, always, even if the provider just used the default value. Making this strict might be a way to keep the API simple and usage clear.

acdlite · 2017-12-06T21:22:06Z

Sorry folks... I hit a weird GitHub bug. GitHub doesn't recognize changes to this PR since I switched the repo from private to public. Going to close and open a new one. Sorry for the disturbance, let's continue over there!

#2

jlongster · 2017-12-06T21:23:21Z

@mjackson Interesting. Personally I was happy to add a top-level MemoryRouter for tests/etc and I thought that was quite elegant. In fact it allowed me to specify the initial state of the Router which was very useful, like <Router initialEntries={[selected]} initialIndex={0}>.

mjackson · 2017-12-06T21:25:49Z

@jlongster You and me both! But it really bothered some people. I can see how the default context value will help with that, though. Now you've got the default value built into the component, instead of needing to get it from another one further up the tree.

gaearon · 2017-12-06T21:26:42Z

Locking since the discussion has moved to #2. Please continue there!

gaearon reviewed Dec 6, 2017

View reviewed changes

acdlite added 3 commits December 6, 2017 09:28

RFC: New version of context

083d7b4

A proposal for a new version of context addressing existing limitations.

Updates based on feedback

dc85c6e

Add section about displayName

02df9cc

pspeter3 reviewed Dec 6, 2017

View reviewed changes

milesj reviewed Dec 6, 2017

View reviewed changes

acdlite mentioned this pull request Dec 6, 2017

New version of context #2

Merged

4 tasks

acdlite closed this Dec 6, 2017

reactjs locked and limited conversation to collaborators Dec 6, 2017


		In the following major release, we'll remove the old API.

		# Unresolved questions

RFC: New version of context #1

RFC: New version of context #1

Conversation

acdlite commented Dec 6, 2017 • edited Loading

Superseded by #2. Please go there instead. I encountered a GitHub bug and had to re-open.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Dec 6, 2017 • edited Loading

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

acdlite Dec 6, 2017 • edited Loading

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

acdlite Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

ryanflorence commented Dec 6, 2017 • edited Loading

acdlite commented Dec 6, 2017 • edited Loading

pspeter3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlongster commented Dec 6, 2017

acdlite commented Dec 6, 2017

Should we warn if a consumer is rendered without a matching provider?

jlongster commented Dec 6, 2017

acdlite commented Dec 6, 2017

acdlite commented Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Dec 6, 2017

jaredpalmer commented Dec 6, 2017

Choose a reason for hiding this comment

mjackson commented Dec 6, 2017

jlongster commented Dec 6, 2017

acdlite commented Dec 6, 2017

jlongster commented Dec 6, 2017

mjackson commented Dec 6, 2017

gaearon commented Dec 6, 2017

acdlite commented Dec 6, 2017 •

edited

Loading

acdlite Dec 6, 2017 •

edited

Loading

acdlite Dec 6, 2017 •

edited

Loading

acdlite Dec 6, 2017 •

edited

Loading

ryanflorence commented Dec 6, 2017 •

edited

Loading

acdlite commented Dec 6, 2017 •

edited

Loading

acdlite commented Dec 6, 2017 •

edited

Loading