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

Use new Context API in React 16.3 #5908

Merged
merged 17 commits into from Sep 20, 2018
Merged

Use new Context API in React 16.3 #5908

merged 17 commits into from Sep 20, 2018

Conversation

@timdorr
Copy link
Collaborator

@timdorr timdorr commented Jan 29, 2018

Update!

OK, a test release is on npm under the next tag.

npm install react-router@next react-router-dom@next

It's just those two packages for now, but please give them a try. Report any problems in this PR.

Continued...

Closes #5901.

This will obviously not work on Travis until 16.3.0 is out. But I can confirm it works locally.

To play along at home, simply clone react in a parallel folder, yarn && yarn run build, and then copy over the files by hand back to this repo:

cp -a ../react/build/node_modules/react/* node_modules/react/
cp -a ../react/build/node_modules/react-dom/* node_modules/react-dom/

This is mostly a brute force approach. All the tests pass, but I'm doing whatever is necessary to get it to work. There may be some more radical refactors warranted with the new API. The new bitmask stuff is neato.

While not BC yet, I'm least keeping available the old context API hooks. I need to feature sniff unstable_createContext.

Outstanding stuff:

  • Wait for a React release (duh)
  • <Redirect>, <Prompt>
  • Router#getChildContext references this.context.router
  • <Link>
  • React Router Native stuff
  • <= 16.2 compatibility
@@ -0,0 +1,5 @@
import React from "react";

const RouterContext = React.unstable_createContext(0);

This comment has been minimized.

@timdorr

timdorr Jan 29, 2018
Author Collaborator

Not sure if {} is a valid default value, but it would be better.

This comment has been minimized.

@pshrmn

pshrmn Jan 29, 2018
Collaborator

I believe it should be the shape of the expected context. Something like this, but I'm not sure how specific it is supposed to be.

React.unstable_createContext({
 history: null,
 route: {
   location: null,
   match: null
  }
});

The <StaticRouter> also places a staticContext on the context, but whether it is necessary to include it I cannot say.

This comment has been minimized.

@acdlite

acdlite Jan 29, 2018
Contributor

It can be whatever you want it to be. null seems like an appropriate choice.

@@ -77,7 +78,10 @@ class Router extends React.Component {

render() {
const { children } = this.props;
return children ? React.Children.only(children) : null;
return RouterContext.provide(
this.getChildContext(),

This comment has been minimized.

@pshrmn

pshrmn Jan 29, 2018
Collaborator

Maybe this should be this.getChildContext().router and then it can be consumed directly.

const Cmp = props => (
  RouterContext.consume(router => <InnerCmp {...props} router={router} />)
);

This comment has been minimized.

@timdorr

timdorr Jan 29, 2018
Author Collaborator

Tried that. Because of how the initial render works under this API, I ran into a boatload of errors.

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Jan 29, 2018

Looks like the RRN stuff is going to have to stick with the old context API (still supported!) for now. Changing it out is fairly easy, but our tests need more extensive changes to support rendering beyond the consumer. And I'm not 100% sure how to do that. Maybe use enzyme? But that's an equally large ask.

Anyways, punting on RRN for the moment.

@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Jan 29, 2018

Enzyme won't work because it needs to do full mounts to render both the provider and consumer. However, from what I can tell, React Native components don't work with Enzyme's mount.

Even with regular React there are Enzyme issues because you can only update the props for the root component. An extra wrapper can probably be added to get around this, but I haven't played around with that yet.

These issues are more "things that don't work now" than "things that wont' work", because there is no reason to expect Enzyme to work with an unstable API, but I figured I'd throw them out there.

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Jan 29, 2018

Who would have thought an experimental feature that landed in an upstream library's master 5 days ago would have trouble being supported by other libraries? /s

@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Jan 29, 2018

*opens window* Its new context API and I need it NOW!

Having used the new context for a bit, did you have any thoughts on how to maintain old context support?

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Jan 29, 2018

Just maintain getChildContext() and use it as your input value to .produce(). That way is easy. It's harder to go back to <= 16.2 support. I don't have an answer for that yet, other than basically ponyfilling the API.

@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Jan 29, 2018

From a maintenance perspective, a clean break is the nicest solution. I have been toying with react-broadcast in my router so that I can mimic (sort of, the API isn't exactly the same) the new context and besides the testing issues everything works well. It might make sense to make that same switch here and transition to the real context whenever that becomes official.

@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Jan 30, 2018

I did some digging into react-test-renderer. I believe that the RN tests should look something like this:

import renderer from 'react-test-renderer';
import { createMemoryHistory } from 'history';

const createHistory = () => {
  const history = createMemoryHistory();
  history.push = jest.fn();
  history.replace = jest.fn();
  return history;
};

describe('...', () => {
  it('...', () => {
    const history = createHistory();
    const output = renderer.create((
      <Router history={history}>
        <Link to='/push' />
      </Router>
    ));

    const event = new EventStub()
    // find the link and "press" it
    const link = output.root.findByType(Link);
    link.props.onPress(event)

    expect(history.push.mock.calls.length).toBe(1)
    expect(history.push.mock.calls[0][0]).toBe('/push')
  });
});
@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Feb 3, 2018

16.3.0-alpha.0 was released with Context.provide/Context.consume replaced by <Context.Provider> and <Context.Consumer>.

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Feb 3, 2018

Welp, that's a little bit of work to implement. But now I can get Travis to pass 👍

@pshrmn
Copy link
Collaborator

@pshrmn pshrmn commented Feb 3, 2018

Yeah, the lack of an href-like attribute on React Native links makes testing where they will navigate to a bit convoluted. The above example could probably be a bit shorter because the history instance only really needs to be mocked if you're verifying the navigation happens.

I'm excited for 16.3. They also dropped by unstable_ prefix on createContext, so hopefully that is a sign of confidence.

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Feb 6, 2018

OK, I added in create-react-context to polyfill for React <= 16.2. I used that to downgrade back to the current stable release, which ensures RRN is in sync.

Lerna's hoisting, combined with the symlinking between packages, was causing two versions of React to sneak into the tests. I'm not sure how best to fix that, but the easiest thing is to run against the stable version. It ensures we're backwards compatible as well.

I think we're good to go now. Just need a review and some feedback on how this turned out.

@timdorr timdorr changed the title [WIP] Use new Context API in React 16.3 Use new Context API in React 16.3 Feb 6, 2018
@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Feb 9, 2018

Just so others are aware of the plan/goal here: I'm holding off on merging this into master until we get our next release out. There are a lot of useful things queued up and this change is large enough that I don't want to poison the well on what should be a high quality release.

Afterwards, I'll look to getting this merged and released as a prerelease to allow for everyone to test it and give feedback. I'm most nervous about the component tree changes (a bunch more of stuff will show up with this in place!) and unintended consequences too.

@pravdomil
Copy link

@pravdomil pravdomil commented Feb 22, 2018

@timdorr can we try it now? Is there any prerelease?

@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Feb 22, 2018

You can check out this PR and build it locally.

import React from "react";
import createReactContext from "create-react-context";

const RouterContext = React.createContext

This comment has been minimized.

This comment has been minimized.

@timdorr

timdorr Apr 30, 2018
Author Collaborator

Yep, this changed since the last time I touched this PR. Since I have to resolve a bunch of package-lock.json file conflicts anyways, I'll get this swapped out at that point.

@techniq
Copy link

@techniq techniq commented May 31, 2018

@timdorr any outstanding issues left for this PR?

@timdorr timdorr force-pushed the react-context-api branch from 6f7e912 to 35c35af Jun 6, 2018
@timdorr
Copy link
Collaborator Author

@timdorr timdorr commented Jul 4, 2018

No API changes needed. The only thing is a new RouterContext export, but it should be fully BC, even with legacy Context.

@bdwain
Copy link

@bdwain bdwain commented Aug 3, 2018

@timdorr do you have any info on how long you want to keep this in alpha before getting ready to release it?

@DeanBDean
Copy link

@DeanBDean DeanBDean commented Aug 13, 2018

Hey @timdorr, is there any way you can push an update for an alpha branch of the react-router-config to include the alpha branch of react-router? I was trying to test these changes, but unfortunately we use react-router-config and I believe it's using the non alpha version of react-router.

EDIT:
Turns out the issue I was seeing was deeper than just the version of react-router in react-router-config. renderRoutes references Switch and Route from react-router. Unfortunately, if we use renderRoutes on the clientSide, we start getting errors that say You should not use <Switch> outside a <Router>. The source of these errors is that BrowserRouter from react-router-dom and Switch/Route from react-router have different RouterContexts. I was able to work around this by creating a version of of renderRoutes that referenced Switch and Route from react-router-dom.

This seems like a bit of a sticky issue. I am not sure if you are partial to any one way of tackling this. If you have something in mind, I don't mind tackling it in a branch of this one to merge into this PR.

Copy link
Member

@mjackson mjackson left a comment

Looks pretty good, @timdorr 😅 Thanks for taking this on!

@@ -7,6 +7,7 @@ export Prompt from "./Prompt";
export Redirect from "./Redirect";
export Route from "./Route";
export Router from "./Router";
export RouterContext from "./RouterContext";

This comment has been minimized.

@mjackson

mjackson Sep 20, 2018
Member

Let's not export RouterContext as part of our public API. It's an implementation detail that users shouldn't care about. If they want routing data, they can always render a <Route> or use withRouter.

This comment has been minimized.

@alexeyraspopov

alexeyraspopov Sep 20, 2018

@mjackson, what do you think about exporting the context's <Consumer /> as a render-prop version of withRouter HOC? withRouter brings addition effort since the piece that needs the data from the router needs to be moved to a separate component that uses the HOC.

This comment has been minimized.

@timdorr

timdorr Sep 20, 2018
Author Collaborator

@alexeyraspopov <Route> has a render prop API, both with the render prop and a children-as-a-function "prop".

This comment has been minimized.

@alexeyraspopov

alexeyraspopov Sep 20, 2018

Oh, I've just found out that path is not required. Worth trying, thank you.

This comment has been minimized.

@crobinson42

crobinson42 Sep 24, 2018

@mjackson what about a case where an application is using a Router nested inside another Router and the nested Router contains a NavLink that would like to use the parent Router's Context... if the Context is exported it can be explicitly used in this situation. Thoughts?

This comment has been minimized.

@josephcsible

josephcsible Sep 25, 2018

@crobinson42 Is such a thing possible today with the old context system? If not, isn't that out of scope for this PR?

This comment has been minimized.

@crobinson42

crobinson42 Sep 25, 2018

@josephcsible I have not been able to find a solution using the old context system. I suppose it would be considered in-scope since this PR would provide the opportunity to use the library in a new way by the benefits of the new context api. But, I do see your point in the requirement it would add to then add a prop to the wrapping <Router context={SpecificRouterContext}> component.

@@ -23,7 +21,7 @@ class Router extends React.Component {
getChildContext() {
return {
router: {

This comment has been minimized.

@mjackson

mjackson Sep 20, 2018
Member

No need to use the router key here. Let's just put everything directly on the context object.

This comment has been minimized.

@timdorr

timdorr Sep 20, 2018
Author Collaborator

This is to maintain BC with those that are relying on our existing legacy Context usage.

@@ -50,17 +51,14 @@ describe("A <Router>", () => {

describe("context", () => {

This comment has been minimized.

@mjackson

mjackson Sep 20, 2018
Member

Let's go ahead and remove all tests that test our context values in this PR. Since context is not part of our public API, we shouldn't have them.

@@ -3,6 +3,7 @@ export Prompt from "./Prompt";
export Redirect from "./Redirect";
export Route from "./Route";
export Router from "./Router";
export RouterContext from "./RouterContext";

This comment has been minimized.

@mjackson

mjackson Sep 20, 2018
Member

Again, let's not expose our context API as public API.

This comment has been minimized.

@timdorr

timdorr Sep 20, 2018
Author Collaborator

We need to export it to get it between the react-router and react-router-dom modules.

@@ -9,6 +9,7 @@ export {
Redirect,
Route,
Router,
RouterContext,

This comment has been minimized.

@mjackson

mjackson Sep 20, 2018
Member

Should be able to remove this as well.

timdorr added 16 commits Jan 29, 2018
@timdorr timdorr force-pushed the react-context-api branch from 8498f91 to 6124f69 Sep 20, 2018
@mjackson mjackson merged commit 449a528 into master Sep 20, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mjackson
Copy link
Member

@mjackson mjackson commented Sep 20, 2018

Thanks, @timdorr! I'm going to make a few tweaks and go ahead and cut a 4.4 beta release with these changes.

@mjackson mjackson deleted the react-context-api branch Sep 20, 2018
@frehner frehner mentioned this pull request Sep 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet