Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New version of context #2

Merged
merged 4 commits into from
Jan 25, 2018
Merged

Conversation

acdlite
Copy link
Member

@acdlite acdlite commented Dec 6, 2017

(supersedes #1)

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

Rendered

Corresponding React PR: facebook/react#11818

TODO:

  • Implementation notes
  • High-priority version of context, for animations. (May submit this separately.)
  • Add snippet showing how the displayName transform would work (New version of context #2 (comment)).
  • Add section about using narrow context types for maximum performance

@acdlite acdlite mentioned this pull request Dec 6, 2017
2 tasks
@jaredpalmer
Copy link

@mjackson compareValues is a good idea. What's the default comparison in RB? ===?

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

@jaredpalmer What's the use case for compareValues? I don't see one except to make mutation easier, which we're specifically aiming to discourage.

@mjackson
Copy link
Member

mjackson commented Dec 6, 2017

@jaredpalmer Yep. === is the default.

@acdlite There's a difference between discouraging mutability and making it impossible to use, IMO. Seems like exactly what defaultProps are for: by default we'd prefer you don't mutate stuff. But if you need to, just override compareValues. AFAICT immutability isn't actually enforced anywhere else in React. One use case is passing around location objects from the current history API, which is mutable.

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

@mjackson Mutation is not impossible with this proposal, it's just slightly less convenient. You can mutate one level down, just not the outermost container. Same as setState API.

@jaredpalmer
Copy link

jaredpalmer commented Dec 6, 2017

Just giving an escape hatch to fallback to. I can see why this should be discouraged. However, mutation did come up in Formik the other day where I wanted to alter something within context if a certain declarative component existed in the subtree.

Edit: Ahh I see now that you can mutate one level down

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

@mjackson

AFAICT immutability isn't actually enforced anywhere else in React.

Historically we have tried to be as forgiving of mutation as possible. When we introduce async rendering (this proposal is designed with async in mind), we may need to be a bit more restrictive.

However, there are cases where React already privileges immutability. setState is one. Another is that we bail out on React elements if the props objects are strictly equal (===). Often that isn't the case, because props objects are created in the render method as part of JSX / React.createElement. But it is the case for components that render this.props.children. If the component re-renders, but the parent didn't, the child elements will bail out on the unchanged props.


# How we teach this

This would be our first API that uses render props. They are an increasingly
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"render props" and "function children" are different; no matter how they're implemented, children aren't just a normal prop - they're special (the third argument to createElement wins over a "children" prop, for example).

Please don't encourage function children.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't encourage function children.

@ljharb Can you explain why this shouldn't be encouraged?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paularmstrong One argument would be that children is almost always used as an implicit prop, but children as a function encourages using children as an explicit prop, which will lead to a bugs when someone attempts to use both at the same time.

<Thing children={firstFn}>
  woops
</Thing>
React.createElement(Thing, { children: firstFn}, 'woops')
{
  type: Thing,
  props: { children: 'woops' },
  ...
}

Yes, that can already happen, but I've never seen anyone write something like this:

<Thing
  children={
    <Well>
      <This is='awkward' />
    </Well>
  }
>
  ...
</Thing>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, @ryanflorence convinced me that it's easier to teach people a render prop accepts a function than it is to teach folks that the children prop can be a function. So in my teaching and open source I've been moving things over to render exclusively to hopefully make things more cohesive across the community while making it easier for people to pick up.

So, I'm in favor of using a prop called render personally.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree with using render prop instead of children. In any instruction/demos I have given it is much easier for people to wrap their head around a prop called render over children being a function.

Copy link
Member

@gaearon gaearon Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That just depends on how you format this. You don’t have to indent it an extra time.

<MyComponent>
  {x => (
    <div>{x}</div>
  )}
</MyComponent>

That's how Prettier formats it too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, Prettier formats both as 1-liners 😄

<div>
  <MyComponent>{x => <div>{x}</div>}</MyComponent>
  <MyComponent render={x => <div>{x}</div>} />
</div>;

Copy link

@FezVrasta FezVrasta Feb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my 2 cents, if we go with a render prop we may as well drop children all together, no reason to keep two props that serve the exact same purpose...

Just to say, please keep children, if one prefers the explicit prop approach, they can do:

<Foo children={x => x} />

If one does:

<Foo children={x => x}>y</Foo>

I'd first ask myself what am I trying to achieve, and then think

the inner content of a component is just treated as children, so I'm just defining children twice, where the latter will take precedence, like any other prop in React

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to support the part where @FezVrasta wrote "please keep children-as-func". :)

Copy link

@streamich streamich Feb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

## Add displayName argument to createContext for better debugging

For warnings and React DevTools, it would help if providers and consumers
had a `displayName`. The question is whether it should be required. We could
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they have a function name, they don't need a displayName - perhaps making getComponentName(…) required, but not displayName specifically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original point is that with this API (unlike existing one) we don’t know which context is missing to show a meaningful warning. Passing a name (explicitly or implicitly) to createContext would solve that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fair, I'm 100% fine with requiring a name - just not requiring displayName specifically, since normal function names should be more than sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createContext() isn’t bound to any component or function in particular so it can’t read a .name from it. Therefore my proposal is to make it an argument (even if optional). I think that’s what displayName was referring to. For example:

const Theme = React.createContext('light', {
  displayName: 'Theme’
});

Sorry if I’m missing your point.

Copy link
Member Author

@acdlite acdlite Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is

const Theme = React.createContext('light');
Theme.displayName = 'Theme';

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon ah, I misunderstood. In that case, yes, I'd say requiring displayName makes perfect sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static patterns like:

const Theme = React.createContext('light');
Theme.displayName = 'Theme';

can often prevent tree-shaking/dead code elimination. I'd advise not to recommend this officially, better to have an optional extra param for createContext + maybe auto add static displayName with babel plugin for the development purposes.


## Other

* Should the consumer use `children` as a render prop, or a named prop?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named prop, please. children are special, and should only be nodes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for render

Copy link

@jamesplease jamesplease Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children are special, and should only be nodes.

Would you mind elaborating on this a little more?

One thought I have is that a new prop could be considered as API bloat if children already functions exactly as it needs to. I'm not convinced one way or the other on this one, by the way; I'm just sharing one more thing worth considering.

Copy link

@JNaftali JNaftali Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type signature for the children prop is pretty consistent across all built-in React components (that I can think of) and most components shipped with third party libraries - Component | SFC | string | etc. The two big exceptions that I've seen are components that don't support children at all and components that require a single child component (React Router's FooRouter components spring to mind).

In particular I think the distinction between (foo) => <DomElement /> vs ({foo}) => <DomElement /> and {renderProp} vs <FunctionalComponent /> is a fine one. I could see that getting confusing in particular if the render prop function isn't defined inline.

All that is why I prefer an explicit 'render' prop, anyway. My knowledge of the React ecosystem is not nearly as in-depth as many of yours' - are there lots of big exceptions that I'm not thinking of?

Edit to add: this is all a user's perspective, not a package author's. If you're still gonna try and discourage users from using the new Context API all of the above might be considered a plus.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me, @JNaftali . I’m now leaning more in favor of a separate prop as well.

Copy link

@stryju stryju Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can always pass children as prop:

<Foo children={() => {...}} />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb Components have always been free to do whatever they want with children. I would agree with you if React by design always passed through children unaltered, but there are so many library components out there that conditionally render children, render them inside wrappers (e.g. interactive rearrangeable grids), etc. that I think "children are special" is a dangerous argument that leads to restricting the power of React.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, saying that children should only be nodes because they are "special", or because the third argument to createElement wins over a "children" prop, are non-arguments. They don't really explain what if anything you consider cleaner, easier to read, easier to understand, less error prone, etc. about ensuring that children are only nodes.

Copy link

@JNaftali JNaftali Dec 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how a convention could possibly 'restrict the power of React'. Conventions don't stop you from doing anything. They're just a tool to make it easier for other people to understand how to use the things you write. React's power is such that given any one of these APIs it'd be easy to implement any of the others that will remain true no matter which of these APIs get implemented.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it's not clear to me if @ljharb is just advocating this as a convention or if he would be in favor of dropping support for function children from React.

As far as making things easier to understand...personally I find child functions more obvious than render functions in other props, but I guess I'm in the minority.

## Other

* Should the consumer use `children` as a render prop, or a named prop?
* How quickly should we deprecate and remove the existing context API?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very slowly. Separately, it must be possible to write a component that can work, simultaneously, on old React (with current context) and new React (with new context).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point about libraries needing to support both APIs at the same time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation wise could we only leave the new implementation but polyfill the old API on top of it (with current broken semantics)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh maybe! Should at least consider it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as I can trivially author a package that can work in both, I consider this addressed :-)

@baptistemanson
Copy link

We didn't use context that much and decided to use Redux instead.

To provide some inputs, I scanned one module. If it helps:

  • out of our 19 redux-connected components, we have 12 components that have only one value selected from the state, aka one selector. Mostly, isLogged, isLoading or isOnline. This proposal totally covers this case elegantly.
  • The remaining 7 components have an average of 4 selectors, to a max of 6. Reading the code, they all seem legitimate to me but we may have got it wrong: isLogged, isOnline, isLoading, getItemsPage, getTotalOfItems, getTheme.
    The new context still sounds laborious in that case.

@gaearon
Copy link
Member

gaearon commented Dec 6, 2017

To be clear even if you use Redux, React Redux still uses context under the hood. The problem is it has to reimplement a tree-aware subscription mechanism. And that won’t work very well with React async mode. The proposal is both about making context friendlier for direct use and to make it better for libraries built on top (such as React Redux).

@milesj
Copy link

milesj commented Dec 6, 2017

(Copied from previous PR)

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

<Consumer onChange={this.handleChange} />

@jlongster
Copy link
Member

@milesj brings up a good point. It took me a bit to parse his question but I think he's getting at this: what if you need the context value in any of the lifecycle hooks?

The problem with a render prop API is it makes that very difficult; I've run into this problem with other render prop-based APIs as well. Take AutoSizer which gives you the width/height. I needed to do something special with the width to figure out if I should render something or not, and that logic needs to be re-run when something else changes in a lifecycle, so I need the width there. Hope that makes sense, I can't remember the details of the problem.

Is the suggested pattern to pull out all the lifecycle code that needs to depend on it into a sub-component, and pass the value down as a prop to it? I think that works but seems a bit ceremonious.

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

@jlongster

Is the suggested pattern to pull out all the lifecycle code that needs to depend on it into a sub-component, and pass the value down as a prop to it? I think that works but seems a bit ceremonious.

Yes, this is the main problem with render prop APIs. I think this is mostly an educational challenge, though, because there's always the option to wrap it in an HOC:

function consume(Context) {
  return Component => {
    return function Consumer(props) {
      return (
        <Context.Consumer>
          {context => <Component context={context} {...props} />}
        </Context.Consumer>
      );
    }
  }
}

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

Usually my objection to wrapping in an HOC, versus using an HOC from the start, is it requires two extra components instead of one. But in this case, we want the extra Consumer component because they're faster to scan for.

@jlongster
Copy link
Member

Faster to scan in what case? Is that benefit enough to warrant a HOC in the default API?

(Thanks for explaining!)

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

Faster to scan for Consumer components. By adding another node into our internal tree, with a special type that is distinct from class components, we can quickly skip over non-consumer nodes.

To be clear, we could still implement it this way with an HOC-first API. I was just rebutting my own devil's advocate point about adding extra nodes to the tree :D

EDIT: Realized I misread your question. Faster to scan for Consumer components when a provider changes. We don't do this every time there's a change, only on the first update. Then the result is cached. We may have to re-scan if the cache is cleared.

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

I'm working on adding a high-level description of the implementation, will try to publish later today or tomorrow.

@oliviertassinari
Copy link

Will the new API support provider nesting like we can do it today? One use case example is nested theme.

@acdlite
Copy link
Member Author

acdlite commented Dec 6, 2017

@oliviertassinari Yes

@milesj
Copy link

milesj commented Dec 7, 2017

@jlongster That's half of the question. But basically, I think the new context would need to hit these points:

  • New life-cycle event(s) for when the context changes (componentWillReceiveContext)? Passing the context prop to a component, in which that component needs to componentWillReceiveProps, adds another layer of abstraction and feels very tedious/contrived. My rudimentary solution above was simply a prop handler for when the context changes, instead of a new life-cycle method.
  • Checking the context value in existing life-cycle events. Theoretically this can be achieved by accessing the context prop.

container. (Or even just alternate between two copies.) React will detect a new
object reference and trigger a change.

## Only one provider type per consumer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone will inevitably develop a component to do this composition automatically. I'm not worried about this drawback 👌

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and while I'm not super excited about the (below) suggested layering I'm certain that if one wasn't available I'd simply make one for use in apps to layer them together ala <ApplicationKitchenSinkConsumer> or something like that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you about the tooling, it will happen because it is necessary.
Inspectors, stack trace and performance may be impacted by the layering though.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly doubt this will happen all that often... 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider that we can change the DevTools to display this however we like

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdlite Indeed! DevTools improvements for HOC/layering makes sense beyond this RFC I think.

@kentcdodds I grep'ed the code of our apps this afternoon to review the impact of this RFC. We only have about 25 developers working with React, so consider my feedback with a grain of salt - we do not have the same stats as Facebook has :).
Our login button render method changes when logged or not authenticated (login or logout), on the theme (color), on being connected to the network or offline (disabled) and on the language of the user.

Overall, 20% of our context dependent render methods read 2 or more "atomic global state variable". If I understood correctly, the RFC suggests that if we want to only use React with no 3rd party tool, we should use 1 Context Consumer for each Context Providers we want to "consume from" - it sounds inconvenient to me.
But we already tooled ourselves with Redux and the RFC doesn't degrade our Redux experience so if it solves problems for others, go!

What about your React applications?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most applications don't use the context API directly in every component that needs it and instead use an abstraction of some kind (HOC, render prop, or something else) to access context. In this case, the impact of the APIs verbosity is pretty minimal and the benefits of the new API make it easily worth it.


# How we teach this

This would be our first API that uses render props. They are an increasingly

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, @ryanflorence convinced me that it's easier to teach people a render prop accepts a function than it is to teach folks that the children prop can be a function. So in my teaching and open source I've been moving things over to render exclusively to hopefully make things more cohesive across the community while making it easier for people to pick up.

So, I'm in favor of using a prop called render personally.


This would be our first API that uses render props. They are an increasingly
popular pattern in third-party React libraries. However, there could be learning
curve for beginners.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't deny the learning curve. But to be fair, there's a learning curve (and arguably a steeper one) with the current context API. In addition, I think people will start to see render prop APIs before they have to start using context themselves. So it's unlikely that people will come to the context API before they've experienced a similar API with other libraries first.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @kentcdodds, context has a learning curve. I think beginners coming across the proposed context API, making use of a render prop, will have a much easier time grokking what is happening, or where to start with it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger learning curve for the existing context API is trudging through myriad opinions about whether or not it's a good idea to use it, and, if so, how to use it safely.

This proposal being slightly more opinionated seems like it would result in a more gradual learning curve.


For warnings and React DevTools, it would help if providers and consumers
had a `displayName`. The question is whether it should be required. We could
make it optional, and use a Babel transform to add the name automatically. This

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make it required, but have the babel plugin add the displayName in the function call automatically? Best of both worlds?

const ThemeContext = React.createContext('light')

   babel plugin   

const ThemeContext = React.createContext('light', {
  displayName: 'ThemeContext',
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I was trying to communicate. Maybe I'll just copy your snippet :D

I was thinking the displayName property would be assigned to the context object directly, as with other component types:

const ThemeContext = React.createContext('light')

   babel plugin   

const ThemeContext = React.createContext('light');
ThemeContext.displayName = 'ThemeContext';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine. I thought making it part of the createContext call would make it easier to log an error. I don't really care how it's implemented though 😅

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to make the plugin to output static property, please make it in such fashion

const ThemeContext = React.createContext('light');
if (process.env.NODE_ENV === 'development') {
  ThemeContext.displayName = 'ThemeContext';
}

Otherwise statics will prevent tree-shaking. This could be configurable with wrap option for the transform which would default to true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, rather,

const ThemeContext = React.createContext('light');
if (process.env.NODE_ENV !== 'production') {
  ThemeContext.displayName = 'ThemeContext';
}

to match how we do other checks.

return (
// The Consumer uses a render prop API. Avoids conflicts in the
// props namespace.
<ThemeContext.Consumer>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a few of our internal apps started using redux in a way such that they do not connect leaf components but instead just rely on the store being accessible on this.context and they grab things from there directly. I'm not saying this is a recommended approach just that it is one I've seen taken.

Given that behavior (or other direct context access and not using a context -> props hoist component) what would be the migration story for apps wanting to update and needing to replace that with the new <ThemeContext.Consumer>?

I realize that's a bit of a loaded question, but it seems like it wouldn't necessarily be something that could be code shifted at all, I imagine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, the migration to using connect()+props instead of reading straight out of context would be my first investigation.
A grep like:

$grep -hr this\.context src/

should give you an idea of the amount of effort required for this direction. Hope that helps!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion around how long existing API would hang around, and implementation for that can be found #2 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you give a static value (like store) to a context provider, consuming it in a child is basically free. So you can continue to use it for the this.context.store use case. The only difference is you'll have to use the render prop, instead of accessing it on the instance. Unfortunately this means there's no obvious way to codemod this, but it shouldn't be hard for a human to do the migration.

@suchipi
Copy link

suchipi commented Dec 7, 2017

It might be nice for React to ship an "official" HOC for consuming Context in the lifecycle, so that it can be treated differently in React DevTools (different color or etc). It's already inconvenient to dig through a ton of layers of components when using context-based APIs; needing to add another layer to get context in lifecycle methods will exacerbate the issue.

static values is still supported.

Replacing subscription-based patterns in favor of context's built-in change
propgation may require a larger effort. However, as a first step, developers can

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo: propagation

the previous context, and return true if they are different. The problem is
that, unlike props or state, we have no type information. The type of the
context object depends on the component's position in the React tree. You could
perform a shallow comparion of both objects, but that only works if we assume

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo: comparison

(if you plan to do a spellcheck at some point, no worries; I can stop pointing these out. I'm just reading through this document and pointing them out as I find them. By the way, this is great so far! Thank you! ✌️ )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha your spellchecks are appreciated :)

mjackson added a commit to ReactTraining/react-broadcast that referenced this pull request Jan 18, 2018
This is an API overhaul to more closely match the API currently being
proposed in reactjs/rfcs#2

The main goals of this work are:

- Conform more closely to the upcoming context API, to make it easier
  for people to migrate off react-broadcast when that API eventually
  lands
- Remove reliance on context entirely, since eventually it'll be gone
- Remove ambiguity around "channel"s

The new API looks like:

    const { Broadcast, Subscriber } = createBroadcast(initialValue)

    <Broadcast value="anything-you-want-here">
      <Subscriber children={value => (
        // ...
      )} />
    </Broadcast>

Instead of providing pre-built <Broadcast> and <Subscriber> components,
we provide a createBroadcast function that may be used to create them.

See the README for further usage instructions.
@acdlite acdlite merged commit b091fdb into reactjs:master Jan 25, 2018
@streamich
Copy link

This proposes an interface that can be done entirely in a 3rd party library. Then why include context in react at all?

import {Provider, Consumer} from 'react-context';

<Provider value={123}>
  <Consumer>{value => /* ... */}</Consumer>
</Provider>

React is already too big, better separate it in another lib, like you did with ReactDOM and PropTypes.

@TrySound
Copy link
Contributor

@streamich This proposal can be implemented as 3rd party library only with current context implementation which is not lying in async rendering idea and will be removed in v17. There's still should be parent/child relationship.

@TrySound
Copy link
Contributor

@streamich Also there is 3rd party polyfill create-react-context

@streamich
Copy link

streamich commented Jan 25, 2018

@TrySound Can't you keep track parent/child relationship in a third party library as well?

@TrySound
Copy link
Contributor

@streamich I can't. Can you?

@taion
Copy link
Member

taion commented Jan 25, 2018

@acdlite

First off, great job on the implementation. It looks really nice, and I actually like it more than what's on this RFC – it fully addresses my concerns here.

However, the implemented API is moderately different from the API proposed here. Can you comment on the bearing this has on the way the RFC process is supposed to work?

There was no "final comment period" per https://github.com/reactjs/rfcs#what-the-process-is, and no mention was made of the changes to the implemented API here.

I have no problems with the end result, but the process here doesn't seem to have followed what was described.

@gaearon
Copy link
Member

gaearon commented Jan 25, 2018

I think the plan was to land the change now so we can start testing it internally, but hold off the final API decision until the actual release. But yeah, this means RFC should’ve probably stayed open until we do that.

@taion
Copy link
Member

taion commented Jan 25, 2018

Great. Shall we reopen then?

@taion
Copy link
Member

taion commented Jan 25, 2018

Oops, never mind. Guess it’d have to be a new PR.

@gaearon
Copy link
Member

gaearon commented Jan 25, 2018

I'll bring it up that we should do this for future RFCs (or amend the process description). I don't think it makes sense to introduce more bureaucracy now that it's already merged though.

@acdlite
Copy link
Member Author

acdlite commented Jan 25, 2018

@taion In my mind, merged means "accepted" but not necessarily "completed." Thanks for the feedback! If you have other comments please open a PR against the existing document. I'm sure we'll get the hang of this process eventually :D

@ljharb
Copy link

ljharb commented Jan 25, 2018

Internal testing can be done off an unmerged branch though, no?

@gaearon
Copy link
Member

gaearon commented Jan 25, 2018

That's pretty hard with our continuous release model. We prefer to always land changes in master and hide them behind feature branches instead.

@taion
Copy link
Member

taion commented Jan 25, 2018

I think @gaearon's point is right. We should have just left the PR open while the strawman implementation was around, then gone through the "final comment period" and all the rest before accepting the RFC and shipping the new code.

mjackson added a commit to ReactTraining/react-broadcast that referenced this pull request Feb 1, 2018
This is an API overhaul to more closely match the API currently being
proposed in reactjs/rfcs#2

The main goals of this work are:

- Conform more closely to the upcoming context API, to make it easier
  for people to migrate off react-broadcast when that API eventually
  lands
- Remove reliance on context entirely, since eventually it'll be gone
- Remove ambiguity around "channel"s

The new API looks like:

    const { Broadcast, Subscriber } = createBroadcast(initialValue)

    <Broadcast value="anything-you-want-here">
      <Subscriber children={value => (
        // ...
      )} />
    </Broadcast>

Instead of providing pre-built <Broadcast> and <Subscriber> components,
we provide a createBroadcast function that may be used to create them.

See the README for further usage instructions.
@reactjs reactjs locked as resolved and limited conversation to collaborators Feb 2, 2018
@gaearon
Copy link
Member

gaearon commented Feb 2, 2018

The RFC has been finalized and implemented (with a functional children prop).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.