React Router v4 support #201

v12 · 2017-02-09T13:47:23Z

So, React Router is now in the beta stage and no major changes are expected to its API.

Here is my attempt to implement RR v4 support 😄

Addresses #186

taion · 2017-02-09T16:28:46Z

src/LinkContainer.js


-    return React.cloneElement(React.Children.only(children), props);
+    return (
+      <Route


Based on official example. children function is called even if current URL doesn't match. It's called with an object that contains a match and a history properties but match will be null if there was no match. Thus it becomes possible to find out if activeClassName and/or activeStyle should be applied to the container.

that is...wonky

@jquense We have no choice here - it's the way RRv4 works. I could've used matchPath here but it'd be reinventing the wheel because <Route> already does the job.

sure, sorry it wasn't a criticism of the code, just the RR api choice

JesseObrien · 2017-03-18T00:50:00Z

Is there any more activity on this? I'd like to make use of this so I don't need to hack things into my current v4 project.

Apparently, it now uses react-router-dom package, which is designed to be used within browser environments

v12 · 2017-03-20T10:46:39Z

Just updated this PR to use the latest version of React Router (v4 is now officially released, hooray 🎉)

MHarris021 · 2017-03-23T13:41:40Z

Can we get this pulled soon please. I'm writing an application for our support team that requires react router v4 and this issue is delaying development.

jquense · 2017-03-23T14:02:41Z

@react-bootstrap/collaborators anyone use RRv4 who can review this? neither @taion or I use it.

Speaking of which does anyone want to maintain this a bit more actively?

MHarris021 · 2017-03-23T17:08:44Z

@taion
I just tested out the LinkContainer code and it fails without supplying the context on creation. Any guide or comments on how to it correctly?

this is the block that throws the error:

static contextTypes = {
        router: PropTypes.shape({
            push: PropTypes.func.isRequired,
            replace: PropTypes.func.isRequired,
            createHref: PropTypes.func.isRequired,
        }).isRequired,
    };

this is used by the render method when rendering the <Route> component

v12 · 2017-03-23T17:21:34Z

@MHarris021 seems you're using not the latest commit because contextTypes prop doesn't match - it should be like this

MHarris021 · 2017-03-23T19:52:10Z

@v12

Thank you. I got it working by making that exact change in the source code I copied from you. I also had to change the call this.context.router.createHref(... to this.context.router.history.createHref(...

It is now rendering properly and my children wrapped in <LinkContainer> are properly having their hrefs changed correctly.

@v12 thank you for putting this together

techrah · 2017-03-30T13:51:42Z

@jquense I just tested @v12's changes by wrapping a NavItem with a LinkContainer and it works! I tested this by installing the current version with npm install react-router-bootstrap, parsing LinkContainer.js and IndexLinkContainer.js through babel and replacing those files in lib/. I did not need to make any additional changes regarding createHref as @MHarris021 mentioned.

Based on the manual tests done so far and that the CI tests have passed, is this enough to accept and merge this PR? If not, what else can we do to help? As the community moves to RRv4, we're really going to need this asap.

Big thanks to everyone for all the hard work!

techrah · 2017-04-03T18:13:50Z

@mtscout6 @taion Who is the current maintainer of this project now? I really need this change, please!

yarrumretep · 2017-04-04T15:23:20Z

+1 for this PR

taion · 2017-04-04T16:39:34Z

To reiterate, where we stand right now is that none of the current active maintainers use RRv4. The code looks fine as far as we can tell, but we can't commit to maintaining this.

We'd be happy to add one or more persons as a maintainer, but we don't really have a path forward here without that.

jjinux · 2017-04-04T18:31:40Z

Hey, @taion, I'm from Udemy. I'm in the process of moving us to RRv4. I don't really know the react-router-bootstrap codebase, and so I don't know how good the patch is. However, if you're looking for people who plan on using this in production, I'm one of them.

taion · 2017-04-04T19:55:43Z

The code here is all there is. There's nothing to this library other than <LinkContainer>. But someone has to be on the hook for e.g. fixing bugs as they come up, and it can't be any of the existing maintainers.

jjinux · 2017-04-04T22:10:32Z

I can do it. Or, we can have @zloiroc (the guy who did core-js) do it since he also works for Udemy. What about @v12--he was the one who submitted the patch?

I assume we should start a new major version number, and there should be some documentation about which version to use based on which version of react-router you're using. New bugs for the RRv3 version can be made against a release branch.

taion · 2017-04-04T22:15:34Z

ping @v12 @zloirock

zloirock · 2017-04-04T22:32:50Z

@taion @jjinux interesting, I'll take a look on this project tomorrow.

taion · 2017-04-05T01:24:13Z

thanks. either way, ideally we'd like to have a couple of people who can maintain this just in case something comes up, since the current maintainers aren't going to be able to do so

v12 · 2017-04-05T09:13:00Z

@taion sure, I can do that. RRv4 is used in a few huge apps we have developed, so react-router-bootstrap is definitely something I'm going to rely on for a long time. But I also do agree that there should be at least a couple of people in case I'm not available. Moreover, it's always good to have another fresh pair of eyes 😉

taion · 2017-04-05T15:05:15Z

Great! Can we get a +1 here from one of you on this code?

techrah · 2017-04-06T13:19:47Z

I have just embarked on a project that will use RRv4 but this is new to me so I am unsure how much help I will be able to provide at the moment but am willing to help out as much as I can. So far, this fix has been working for me but I haven't really put it through its paces yet.

v12 · 2017-04-10T12:29:11Z

Is there still no volunteers to review the PR? 😃

Looking at the activity in this thread as well as in the related issue, would anyone want to have a look at the code? It may look like there are a lot of changes based on changed lines counter, however, most of the changes are related to tests in order to make them compatible with RRv4. Just in case, there is also visual-test script, which can be run to see the library in action, and helps to see that there are no differences between standard React Bootstrap components and them being wrapped with react-router-bootstrap. The latter ones also behave as usual RRv4 links.

Let me know if there is anything else I can do to speed up merge of this PR because, from what I see, it's one of the things that we need as RRv4 becomes more and more frequently used by community. 😉

jjinux

I certainly don't understand every detail, but LGTM.

You need to:

Create a branch for the existing code so that bug fixes and new releases can continue to be made for the code that works for the pre-RRv4 version of the code.
Update the README to say that people using versions of RR earlier than v4 should use ealier versions of this library, whereas people using RRv4 should use such and such a version or later.
Create a new major version number and do a release.

jjinux · 2017-04-10T16:02:46Z

src/LinkContainer.js

+      PropTypes.string,
+      PropTypes.object,
+    ]).isRequired,
+    exact: PropTypes.bool,


Can you give this a default? Same question for anything else that's not required.

Yes, I think it's worth to add default values for exact and strict props that relate to how React Router detects if route is active. However, I don't think it's worth to add default values for other non-required props just because it does not add any value and those may not be defined, so there is no need to pass them down to underlying component.

Obviously, I can set undefined for them in defaultProps but does it make any sense? Seems to just increase amount of code since these props are undefined anyway.

jjinux · 2017-04-10T16:14:17Z

test/visual/ButtonVisual.js


 import LinkContainer from '../../src/LinkContainer';

 export default () => (
  <div>
-    <Link to="home">Back to Index</Link>
+    <Link to="/home">Back to Index</Link>


Is this change something that you need to document in a changelog so that people know that they need to make it to their own code?

Oh, and remember to update the CHANGELOG.md just in general ;)

No, this is not necessary change - I updated URLs just for the sake of clarity as having absolute path looks more clear to me (you definitely know that it points to /home and not some /something/home).

Yes, sure, I'll definitely update changelog 👍

jjinux · 2017-04-10T17:38:11Z

Is there any chance we can get a beta release of this published to npm to facilitate testing?

jjinux · 2017-04-10T18:10:22Z

It's working for me:

v12 · 2017-04-11T11:30:48Z

@jjinux huge thanks for your review!

Regarding major version bump, I'm not sure if this is applicable as react-router-bootstrap is still in unstable stage and, according to SemVer spec (here), API can change unless version is >=1.0.0

Although, if @jquense and @taion agree, then we can go into public release stage by publishing this as 1.0.0-beta.0 first and, after some time, if there are no issues, we can proceed with 1.0.0 release?

jquense · 2017-04-11T12:39:48Z

RB projects follow the general node package convention of treating minor versions <1.0.0 as "major" bumps and patch bumps as patch and minor. you can jump to 1.0.0 if you want, or just bump the minor spot.

jjinux · 2017-04-11T17:31:53Z

I would just release a 0.24.0, but that's just my opinion based on limited knowledge.

MiLk · 2017-04-15T14:01:36Z

Can we also have the release published on npm?
https://www.npmjs.com/package/react-router-bootstrap is still showing 0.23.1

Thank you.

v12 · 2017-04-15T14:10:59Z

@MiLk yes, sure, I've already asked publishers to release it on npm because I don't have access to this package there

taion · 2017-04-15T19:17:12Z

oof, there was a publish script here that handles some of the pub tasks. going to need a bit of time to unwind this – probably not until monday

v12 · 2017-04-15T19:31:53Z

@taion no worries! Have a great weekend 😉

parse · 2017-04-19T05:14:33Z

Did the 0.24.1 release drop support for react-router v3?

taion · 2017-04-19T05:16:36Z

correct – the APIs are totally different so it's impossible to support both.

v0.23.x will continue to work fine with RRv2 and RRv3, though, and will continue to work indefinitely.

@v12 given that LinkContainer.js actually imports from react-router-dom, you should add an explicit peer dependency there

parse · 2017-04-19T05:20:08Z

Gotcha, thanks!

I'm going to be stuck at v0.23.x for a couple of months before I can make the switch. Any plans to release a patch for 0.23.x to address the React proptype deprecation warnings?

taion · 2017-04-19T05:21:21Z

feel free to submit a PR, or we might get to it eventually. we'll continue to cut releases off v0.23.x. for many classes of applications RRv2 and RRv3 remain a better fit than RRv4, so the old releases aren't deprecated or anything.

parse · 2017-04-19T05:37:28Z

@taion I made an attempt here: https://github.com/react-bootstrap/react-router-bootstrap/pull/207/files

taion · 2017-04-19T06:01:01Z

released v0.23.2

note that if you're currently using RRv2 or RRv3, found is probably a better upgrade path than RRv4, and support for this sort of thing is available mostly out-of-the-box there without an extra integration

parse · 2017-04-19T06:01:48Z

Thanks @taion ! Impressed by the speed! :)

In react-bootstrap#201, the switch to `react-router-dom` was made but the webpack config was not updated. Therefore, the current UMD bundles include `react-router-dom` and depend on `react-router`.

Add React Router v4 support

2c28b9d

taion reviewed Feb 9, 2017

View reviewed changes

v12 added 4 commits March 20, 2017 10:22

Fix <IndexLinkContainer> by explicitly setting exact prop to true

e986ece

Use stable release of React Router v4

593b055

Apparently, it now uses react-router-dom package, which is designed to be used within browser environments

Update visual tests to use react-router-dom

016eb9d

Fix code style

946e8c6

Revert change made to <IndexLinkContainer> prop

94062a0

jjinux approved these changes Apr 10, 2017

View reviewed changes

Set default values for exact and strict props

32f9e44

v12 merged commit f771154 into react-bootstrap:master Apr 15, 2017

v12 deleted the rr-v4 branch April 15, 2017 13:38

jochenberger mentioned this pull request Sep 1, 2017

Fix webpack config #225

Merged

renovate bot mentioned this pull request Oct 11, 2018

Update dependency react-router-bootstrap to ^0.26.0 - abandoned neolanders/min-starter-react-redux-axios#15

Open

1 task

React Router v4 support #201

React Router v4 support #201

Conversation

v12 commented Feb 9, 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

JesseObrien commented Mar 18, 2017

v12 commented Mar 20, 2017

MHarris021 commented Mar 23, 2017

jquense commented Mar 23, 2017

MHarris021 commented Mar 23, 2017 • edited Loading

v12 commented Mar 23, 2017 • edited Loading

MHarris021 commented Mar 23, 2017

techrah commented Mar 30, 2017

techrah commented Apr 3, 2017

yarrumretep commented Apr 4, 2017

taion commented Apr 4, 2017

jjinux commented Apr 4, 2017

taion commented Apr 4, 2017

jjinux commented Apr 4, 2017

taion commented Apr 4, 2017

zloirock commented Apr 4, 2017

taion commented Apr 5, 2017

v12 commented Apr 5, 2017

taion commented Apr 5, 2017

techrah commented Apr 6, 2017

v12 commented Apr 10, 2017

jjinux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v12 Apr 11, 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

jjinux commented Apr 10, 2017

jjinux commented Apr 10, 2017

v12 commented Apr 11, 2017 • edited Loading

jquense commented Apr 11, 2017

jjinux commented Apr 11, 2017

MiLk commented Apr 15, 2017

v12 commented Apr 15, 2017

taion commented Apr 15, 2017

v12 commented Apr 15, 2017

parse commented Apr 19, 2017

taion commented Apr 19, 2017

parse commented Apr 19, 2017

taion commented Apr 19, 2017

parse commented Apr 19, 2017

taion commented Apr 19, 2017

parse commented Apr 19, 2017

v12 commented Feb 9, 2017 •

edited

Loading

MHarris021 commented Mar 23, 2017 •

edited

Loading

v12 commented Mar 23, 2017 •

edited

Loading

v12 Apr 11, 2017 •

edited

Loading

v12 commented Apr 11, 2017 •

edited

Loading