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
Member

@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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@timdorr
Copy link
Member Author

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
Contributor

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
Member Author

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
Contributor

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
Member Author

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
Contributor

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
Contributor

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
Contributor

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
Member Author

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
Contributor

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
Member Author

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
Member Author

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 commented Feb 22, 2018

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

@timdorr
Copy link
Member Author

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
Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 commented May 31, 2018

@timdorr any outstanding issues left for this PR?

@jacargentina
Copy link

@timdorr I'm trying to test this with no luck, the API changes in some way? There is some docs on the needed changes? withRouter still works? RouterContext/Provider should i put somwhere? Thanks!

@timdorr
Copy link
Member Author

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 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 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

Choose a reason for hiding this comment

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

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";
Copy link
Member

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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?

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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: {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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", () => {
Copy link
Member

Choose a reason for hiding this comment

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

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";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to remove this as well.

@mjackson mjackson merged commit 449a528 into master Sep 20, 2018
@mjackson
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

None yet