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

Regression: 2.2.0 breaks for server side rendering with code working in 2.0.0 & 2.1.1 #3307

Closed
MoOx opened this issue Apr 14, 2016 · 17 comments

Comments

@MoOx
Copy link

MoOx commented Apr 14, 2016

It's sort of the same issue I mentioned in #3279 but with ReactRouter.match({ routes: routes, location: "/", basename: "/" }` that didn't work anymore (since 2.2.0, same with 2.2.1).

code with 2.0.0 (works with 2.1.1 too) http://jsbin.com/saziwop/edit?html,js,console
code with 2.2.0 http://jsbin.com/zivopi/edit?html,js,console

const App = React.createClass({
  render() {
    return (
      <div />
    )
  }
})

var routes = (
  <ReactRouter.Router>
    <ReactRouter.Route path="*" component={App}/>
  </ReactRouter.Router>
)

ReactRouter.match({ routes: routes, location: "/", basename: "/" }, (error, redirectLocation, renderProps) => {
  if (renderProps) {
    console.log("ok")
  }
  else {
    console.log("NOK")
  }
})

Previously my issue wwas with "/test-url" but now same issue with "/" (and basename to "/")

@taion
Copy link
Contributor

taion commented Apr 14, 2016

I'm taking a look. As a temporary workaround, can you remove the trailing slash from your basename?

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

This code is shipped with phenomic so it's not an easy task atm :) As a workaround I told user to stick to 2.1.1 MoOx/phenomic#393

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

Maybe this commit introduced that regression ac0b761

@taion
Copy link
Contributor

taion commented Apr 14, 2016

I see – this is actually exposing a different issue than the original one. I coincidentally had a fix under #3246, but I didn't cherry-pick it out.

@taion
Copy link
Contributor

taion commented Apr 14, 2016

BTW, I'm not sure if there was some confusion, but you shouldn't be rooting your routes with a <Router>. In the case you have, the minimal config would just be e.g.

<Route path="*">

which wouldn't trigger this regression.

@taion
Copy link
Contributor

taion commented Apr 14, 2016

Is your actual route config:

<Route>
  <Route path="*" />
</Route>

?

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

Yeah. This

<Route component={ LayoutContainer }>
    <Route path="*" component={ PageContainer } />
</Route>

https://github.com/MoOx/phenomic/blob/0e67119ba0c86e195183d3877e60d5ae6ddc472a/boilerplate/web_modules/app/routes.js#L27-L31

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

(In a very near futur, we are going to add more routes, but the first level is to have a common wrapper for all routes. Is there a better solution ?

@taion
Copy link
Contributor

taion commented Apr 14, 2016

This will be fixed by #3308.

I believe this is an edge case triggered by the path being /, when using a route with path="*" nested under a pathless route.

We had a number of test cases for route matching in our test suite, but this specific case wasn't covered. I'm adding a regression test here with #3308 to keep this from happening again.

@taion
Copy link
Contributor

taion commented Apr 14, 2016

Rather, this only happens when / gets matched by a <Route path="*">, nested under a pathless route. If you had some other route in front of the <Route path="*"> that would match the /, then this also wouldn't happen.

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

Ok good to hear. Thanks for your work.

@taion
Copy link
Contributor

taion commented Apr 14, 2016

Sorry this is causing problems. We're tweaking the route matching to be stricter with slashes, to avoid a bug where a route like

<Route path="foo">
  <Route path="bar" />
</Route>

would match a URL like /foo/////////bar/////. The intention of the fix is to make that path config only match /foo/bar and /foo/bar/, and nothing else.

Unfortunately, this requires us to be quite a lot stricter with slashes in paths, and this appears to be revealing a couple of odd edge cases.

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

You are doing the right thing, no problem :)

MoOx added a commit to MoOx/phenomic that referenced this issue Apr 14, 2016
MoOx added a commit to MoOx/phenomic that referenced this issue Apr 14, 2016
MoOx added a commit to putaindecode/putaindecode.io that referenced this issue Apr 14, 2016
@taion
Copy link
Contributor

taion commented Apr 14, 2016

This is fixed on v2.2.2 now.

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

Thanks for fixing this!

@taion
Copy link
Contributor

taion commented Apr 14, 2016

Any more edge cases in our matching code you're hitting? 😛

@MoOx
Copy link
Author

MoOx commented Apr 14, 2016

Haha, we'll see ^^

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants