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

[META] v3.0.0 #3611

Closed
taion opened this Issue Jul 5, 2016 · 34 comments

Comments

Projects
None yet
@taion
Contributor

taion commented Jul 5, 2016

I'm setting this up as a meta-issue instead of a milestone to allow for more commentary and discussion. I'm going to close the originating issues and fold them into here for now, so we have a single issue on which to track things.

We're looking to cut a v3.0.0 soon, starting with PRs, to update dependencies, clean up deprecations from v2.x, and give users a path forward. I'm going to use this meta issue to list out the changes we're looking at making as we lead into release.

Requirements

These are the things that we must resolve before release:

  • Upgrade history dependency to v3 (#3515)
    • If we go ahead with ReactTraining/history#322, we can probably drop the basename enhancer from the singleton histories, and let users use withBasename; given the removal of the automatic <base href> handling, adding the basename enhancer by default doesn't accomplish much
    • Should we force an initial replace to ensure that the initial location has a key when using browser history? cc @mjackson
  • Factor out context subscriber code into separate package (#3484)

Wishlist

These are the things that I'd personally like to get to before release, but am willing to discard if they cause too much schedule slip:

  • Pull out <LocationProvider> component to inject down location as a prop into a separate component that just handles routing state management
    • Would allow better Redux integration, since the matcher could then consume location from the store
    • Might not be possible? How do we interact with listenBefore?
  • Move matching logic into route objects (#2286)
    • Instead of having a top-level matchRoutes handler, instead have the router ask each route for whether it matches, and the matched routes
    • This would allow creating e.g. <HapiRoute>, to allow users to use route handlers with their choice of route matching
    • We could get a proper routeParams too, as each route would give its matched params (#3549)
  • Support returning promises from dynamic methods on routes (getComponent, getChildRoutes, &c.) (#3606)
    • This will allow better support for System.import
  • Remove some edge case handling in default matching logic (with deprecations on v2.x)
    • Remove default support for nested absolute routes whose parents don't match (this can be handled in a custom-matching route) (#3172)
    • Change default pattern handling to path-to-regexp? This would make things easier to learn. We could warn on individual routes where the two matching algorithms give different results
  • Split up client-side and server-side match helpers, since they don't do quite the same thing (#3290)
  • Ensure that context subscriber handling works with context interceptors, as this will be needed for relative link support (but such support proper probably is too ambitious to squeeze into v3.0.0)

Well, please discuss.

The idea is to avoid any major breaking API changes – a working v2.x config should work with minor changes in v3.0.0. I'd like to take the opportunity to clean up cruft, but it's intentional that the wishlist is much longer than the list of requirements – I'm not going to be heartbroken if we can't get to some of these.

@taion taion added this to the v3.0.0 milestone Jul 5, 2016

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jul 5, 2016

Contributor

Oops! Hit "submit" too quickly.

cc @reactjs/routing, @gaearon

The wishlist really is the wishlist. I'd like to get an rc.1 with the upgraded history dep this week if possible, and the rest is just "as we have time before we hit a reasonable release deadline".

Contributor

taion commented Jul 5, 2016

Oops! Hit "submit" too quickly.

cc @reactjs/routing, @gaearon

The wishlist really is the wishlist. I'd like to get an rc.1 with the upgraded history dep this week if possible, and the rest is just "as we have time before we hit a reasonable release deadline".

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jul 5, 2016

Contributor

BTW, "support async route leave hooks" is I think an explicit non-feature here. Users should leverage the custom confirmation prompt support in history to accomplish showing custom dialogs there.

Contributor

taion commented Jul 5, 2016

BTW, "support async route leave hooks" is I think an explicit non-feature here. Users should leverage the custom confirmation prompt support in history to accomplish showing custom dialogs there.

This was referenced Jul 5, 2016

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Jul 5, 2016

Collaborator

Regarding the history v3 upgrade, how do we want to handle that? Should we support both v2 and v3 histories? The API change around location is rather significant, so I know it would break a number of tools (react-router-redux most notably, but the fix is really simple).

Collaborator

timdorr commented Jul 5, 2016

Regarding the history v3 upgrade, how do we want to handle that? Should we support both v2 and v3 histories? The API change around location is rather significant, so I know it would break a number of tools (react-router-redux most notably, but the fix is really simple).

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jul 6, 2016

Contributor

I'm condensing and clearing out some earlier comments:

@dmitriid notes that the withRouter API doesn't match the API for route components in 2.x. This is resolved in 3.0.0-alpha.1, where withRouter also injects routes, params, and location. Thus, the injected props to a withRouter component mostly matches what's available to a route component, net of route, which we would need extra work to be able to inject. We will keep context.router as a supported API, subject to React not changing how context works under us, &c.

@umidbekkarimov mentions push handlers with special query support. We're currently not planning on implementing something along those lines, because the current push API is already quite good for merging and overriding queries:

router.push({ ...location, query: newQuery })
router.push({ ...location, query: { ...query, ...newQuery } })
Contributor

taion commented Jul 6, 2016

I'm condensing and clearing out some earlier comments:

@dmitriid notes that the withRouter API doesn't match the API for route components in 2.x. This is resolved in 3.0.0-alpha.1, where withRouter also injects routes, params, and location. Thus, the injected props to a withRouter component mostly matches what's available to a route component, net of route, which we would need extra work to be able to inject. We will keep context.router as a supported API, subject to React not changing how context works under us, &c.

@umidbekkarimov mentions push handlers with special query support. We're currently not planning on implementing something along those lines, because the current push API is already quite good for merging and overriding queries:

router.push({ ...location, query: newQuery })
router.push({ ...location, query: { ...query, ...newQuery } })
@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jul 6, 2016

Contributor

For history v3, I think the right way to go forward is to do something like this check: https://github.com/reactjs/react-router/blob/v2.5.2/modules/Router.js#L18-L21

The user-facing APIs are largely identical, so we'll just force the user to supply a v3 history, and throw if the history does not support getCurrentLocation.

Contributor

taion commented Jul 6, 2016

For history v3, I think the right way to go forward is to do something like this check: https://github.com/reactjs/react-router/blob/v2.5.2/modules/Router.js#L18-L21

The user-facing APIs are largely identical, so we'll just force the user to supply a v3 history, and throw if the history does not support getCurrentLocation.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Jul 13, 2016

Collaborator

I was going to PR something around that soon. It shouldn't be hard, just change things like this call to getComponents to check for a Promise.

Collaborator

timdorr commented Jul 13, 2016

I was going to PR something around that soon. It shouldn't be hard, just change things like this call to getComponents to check for a Promise.

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Aug 10, 2016

I have the feeling that async onEnter isn't working as expected in general (imho).

Say I have a route-a rendering ComponentA and a route-b rendering ComponentB and switching between both routes takes 2 seconds.

  1. When I switch from route-a to route-b I wouldn't expect that the URL changes immediately, but only if I call the callback inside onEnter. That way I can abort the transition if an error happens or redirect to a different route without changing the URL to a route which is never rendered.
  2. When I switch from route-a to route-b and I go back in my browser history after 1 second, I see the URL for route-a, but ComponentB will be rendered. (This is basically a different scenario for issue #3476.)
  3. Animations don't behave as expected (explained here).

It would be nice if this can be addressed in v3 and if this works as intended, it would be nice if the docs could explain this behaviour.

donaldpipowitch commented Aug 10, 2016

I have the feeling that async onEnter isn't working as expected in general (imho).

Say I have a route-a rendering ComponentA and a route-b rendering ComponentB and switching between both routes takes 2 seconds.

  1. When I switch from route-a to route-b I wouldn't expect that the URL changes immediately, but only if I call the callback inside onEnter. That way I can abort the transition if an error happens or redirect to a different route without changing the URL to a route which is never rendered.
  2. When I switch from route-a to route-b and I go back in my browser history after 1 second, I see the URL for route-a, but ComponentB will be rendered. (This is basically a different scenario for issue #3476.)
  3. Animations don't behave as expected (explained here).

It would be nice if this can be addressed in v3 and if this works as intended, it would be nice if the docs could explain this behaviour.

@yormi

This comment has been minimized.

Show comment
Hide comment
@yormi

yormi Aug 11, 2016

I don't know if it can be useful for you guys but since 3.0.0-alpha.2, the first click on a Link when the app is loaded change the URL but does not render the component associated to the new URL.

After that first click, everything works fine though.

I use react-router-redux, I don't know if it changes something...

yormi commented Aug 11, 2016

I don't know if it can be useful for you guys but since 3.0.0-alpha.2, the first click on a Link when the app is loaded change the URL but does not render the component associated to the new URL.

After that first click, everything works fine though.

I use react-router-redux, I don't know if it changes something...

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Aug 15, 2016

Contributor

Quick update for anyone that cares – main blocker for moving to an actual RC is figuring out how to display loading status when doing async routing, as that's become a major issue for us at work.

Need to figure out the appropriate API here, and I don't think we should commit to an RC until we've figured that out (would ideally like to avoid that sort of API change after we hit RC).

Contributor

taion commented Aug 15, 2016

Quick update for anyone that cares – main blocker for moving to an actual RC is figuring out how to display loading status when doing async routing, as that's become a major issue for us at work.

Need to figure out the appropriate API here, and I don't think we should commit to an RC until we've figured that out (would ideally like to avoid that sort of API change after we hit RC).

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 16, 2016

@taion some thoughts about loading status:

These days I prefer doing async loading in a userland component (that displays loading status) instead of using an async getComponent. IMO getComponent isn't really the best place to do async loading, and it would be better to deprecate it and encourage users to pass in a component that does the async loading (and status display if they want).

On the other hand, getChildRoutes is quite different. I have a userland solution for displaying loading status while async loading child routes too, but it involves several messy hacks. So I think a good API for returning a temporary loading route while the real child routes are being loaded is worth the effort (and I'm guessing the same would be the case for getIndexRoute, though I haven't ever needed to use it).

jedwards1211 commented Aug 16, 2016

@taion some thoughts about loading status:

These days I prefer doing async loading in a userland component (that displays loading status) instead of using an async getComponent. IMO getComponent isn't really the best place to do async loading, and it would be better to deprecate it and encourage users to pass in a component that does the async loading (and status display if they want).

On the other hand, getChildRoutes is quite different. I have a userland solution for displaying loading status while async loading child routes too, but it involves several messy hacks. So I think a good API for returning a temporary loading route while the real child routes are being loaded is worth the effort (and I'm guessing the same would be the case for getIndexRoute, though I haven't ever needed to use it).

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Aug 16, 2016

Contributor

Doing async loading in the component is tempting but suboptimal in the case of nested routes – it forces waterfalled loads, as we've discussed on other issues.

I consider that compromise to be unacceptable. Users should not do async loading as part of the render cycle.

Contributor

taion commented Aug 16, 2016

Doing async loading in the component is tempting but suboptimal in the case of nested routes – it forces waterfalled loads, as we've discussed on other issues.

I consider that compromise to be unacceptable. Users should not do async loading as part of the render cycle.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 16, 2016

Oh man, I keep forgetting about that. How awkward...

jedwards1211 commented Aug 16, 2016

Oh man, I keep forgetting about that. How awkward...

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Aug 19, 2016

Maybe this is an interesting point for an upgrade guide:

  • Since react-router v3 uses history v3 hashHistory has no _k by default, if no location.state is used (see ReactTraining/history#163).
  • Some libraries like taion/react-router-scroll need that key to work correctly, even if location.state isn't used. (Maybe this is a code smell in itself...?)

I'm not really sure currently what is the best way to bring back _k. I guess I need to create a custom hashHistory...?

EDIT:

Maybe the fastest way to get _k back.

hashHistory.listenBefore((location) => {
  if (location.state === undefined) {
    location.state = true;
  }
});

donaldpipowitch commented Aug 19, 2016

Maybe this is an interesting point for an upgrade guide:

  • Since react-router v3 uses history v3 hashHistory has no _k by default, if no location.state is used (see ReactTraining/history#163).
  • Some libraries like taion/react-router-scroll need that key to work correctly, even if location.state isn't used. (Maybe this is a code smell in itself...?)

I'm not really sure currently what is the best way to bring back _k. I guess I need to create a custom hashHistory...?

EDIT:

Maybe the fastest way to get _k back.

hashHistory.listenBefore((location) => {
  if (location.state === undefined) {
    location.state = true;
  }
});
@buzinas

This comment has been minimized.

Show comment
Hide comment
@buzinas

buzinas Aug 20, 2016

I would like to have two other hooks: onBeforeEnter and onBeforeLeave. I needed those hooks in two projects already, and I'm always hacking something to make this behavior possible.

When entering a route, the general idea is:

  • If there is no onBeforeEnter hook set, we keep the current behavior
  • If it's set, it is expected to return a Boolean.
    • If its return value is truthy, we keep the current behavior
    • If its return value is falsy, then we don't call the onEnter hook and we cancel the current behavior.

The onBeforeLeave hook would work in a similar way when leaving a route.

buzinas commented Aug 20, 2016

I would like to have two other hooks: onBeforeEnter and onBeforeLeave. I needed those hooks in two projects already, and I'm always hacking something to make this behavior possible.

When entering a route, the general idea is:

  • If there is no onBeforeEnter hook set, we keep the current behavior
  • If it's set, it is expected to return a Boolean.
    • If its return value is truthy, we keep the current behavior
    • If its return value is falsy, then we don't call the onEnter hook and we cancel the current behavior.

The onBeforeLeave hook would work in a similar way when leaving a route.

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Aug 20, 2016

Contributor

We're not planning on adding additional route hooks at the moment. The API surface area is too large there, and many of the existing use cases are better handled at the component level.

While we don't plan on deprecating any existing route hooks in v3.x, we are unlikely to add any new ones. The set of use cases there are not broad enough to warrant the increased API surface area.

Contributor

taion commented Aug 20, 2016

We're not planning on adding additional route hooks at the moment. The API surface area is too large there, and many of the existing use cases are better handled at the component level.

While we don't plan on deprecating any existing route hooks in v3.x, we are unlikely to add any new ones. The set of use cases there are not broad enough to warrant the increased API surface area.

@zdila

This comment has been minimized.

Show comment
Hide comment
@zdila

zdila Sep 9, 2016

Meanwhile history 4.0.0 has been released ;-)

zdila commented Sep 9, 2016

Meanwhile history 4.0.0 has been released ;-)

@killroy42

This comment has been minimized.

Show comment
Hide comment
@killroy42

killroy42 Sep 13, 2016

But how does it work? :(

killroy42 commented Sep 13, 2016

But how does it work? :(

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Sep 13, 2016

Collaborator

history 4.0 will correspond with react-router 4.0, which is being worked on separately from 3.0. This is more of an evolutionary change, whereas 4.0 will be more revolutionary (and hence history 4.0 is a very big shift).

Collaborator

timdorr commented Sep 13, 2016

history 4.0 will correspond with react-router 4.0, which is being worked on separately from 3.0. This is more of an evolutionary change, whereas 4.0 will be more revolutionary (and hence history 4.0 is a very big shift).

@killroy42

This comment has been minimized.

Show comment
Hide comment
@killroy42

killroy42 Sep 13, 2016

I'm downgrading to 2.8.0. It seems that the 4.x release isn't official. This site https://react-router-website-uxmsaeusnn.now.sh/quick-start looks kinda dodge and the API is a bit of a mess now, aparently... I supose I upgraded pre-maturely. but a -beta or -rc tag would be great.

killroy42 commented Sep 13, 2016

I'm downgrading to 2.8.0. It seems that the 4.x release isn't official. This site https://react-router-website-uxmsaeusnn.now.sh/quick-start looks kinda dodge and the API is a bit of a mess now, aparently... I supose I upgraded pre-maturely. but a -beta or -rc tag would be great.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Sep 13, 2016

Collaborator

It's a pre-release, hence the - after the version. It's "official" in the sense that @ryanflorence released it. It has an entirely new design, so there isn't really a defined upgrade path just yet. You should give it a try on a new application, rather than trying to shoehorn it into an existing app with routing in it.

Collaborator

timdorr commented Sep 13, 2016

It's a pre-release, hence the - after the version. It's "official" in the sense that @ryanflorence released it. It has an entirely new design, so there isn't really a defined upgrade path just yet. You should give it a try on a new application, rather than trying to shoehorn it into an existing app with routing in it.

@killroy42

This comment has been minimized.

Show comment
Hide comment
@killroy42

killroy42 Sep 13, 2016

Understood. I downgraded. Been optimistically updating a lot of things today, Not sure if I spend more time chasing bugs already fixed in more recent releases or new bugs introduced by more recent releases ;)

killroy42 commented Sep 13, 2016

Understood. I downgraded. Been optimistically updating a lot of things today, Not sure if I spend more time chasing bugs already fixed in more recent releases or new bugs introduced by more recent releases ;)

@CrocoDillon

This comment has been minimized.

Show comment
Hide comment
@CrocoDillon

CrocoDillon Sep 13, 2016

I know that this is about v3 but since we're talking about v4 already anyway, is there any place where I can find the reasoning behind the changes in v4? It seems like everything has changed and it would help me (and I'm sure others) to know why.

CrocoDillon commented Sep 13, 2016

I know that this is about v3 but since we're talking about v4 already anyway, is there any place where I can find the reasoning behind the changes in v4? It seems like everything has changed and it would help me (and I'm sure others) to know why.

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Sep 13, 2016

Contributor

Yeah we'll have a write up soon. Short story is, React Router had too much API and got way too far from being a "React Router" but was rather a "Router for React". It wasn't composable, and it wasn't declarative. So we made it composable and declarative.

Contributor

ryanflorence commented Sep 13, 2016

Yeah we'll have a write up soon. Short story is, React Router had too much API and got way too far from being a "React Router" but was rather a "Router for React". It wasn't composable, and it wasn't declarative. So we made it composable and declarative.

@jochenberger

This comment has been minimized.

Show comment
Hide comment
@jochenberger

jochenberger Sep 14, 2016

Contributor

On a side note, https://react-router-website-uxmsaeusnn.now.sh/ contains a dead link ("hosted on GitHub" points to https://github.com/reactjstraining/react-router).

Contributor

jochenberger commented Sep 14, 2016

On a side note, https://react-router-website-uxmsaeusnn.now.sh/ contains a dead link ("hosted on GitHub" points to https://github.com/reactjstraining/react-router).

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Sep 16, 2016

Collaborator

Just merged in master to next and resolved some conflicts there. I think we're good to go up to beta now. Most other things in here can be addressed in a minor.

Collaborator

timdorr commented Sep 16, 2016

Just merged in master to next and resolved some conflicts there. I think we're good to go up to beta now. Most other things in here can be addressed in a minor.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Sep 16, 2016

Collaborator

3.0.0-beta.1 is out! https://github.com/ReactTraining/react-router/releases/tag/v3.0.0-beta.1

npm install react-router@beta to get it!

Collaborator

timdorr commented Sep 16, 2016

3.0.0-beta.1 is out! https://github.com/ReactTraining/react-router/releases/tag/v3.0.0-beta.1

npm install react-router@beta to get it!

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Sep 17, 2016

I'm not on twitter, so I'm posting here, v4 looks awesome! I read some of the examples and rationale today, and the new API makes so much sense. Reminds me how some of my own component designs have evolved as I've gotten more experienced using React. Can't wait to use v4!

jedwards1211 commented Sep 17, 2016

I'm not on twitter, so I'm posting here, v4 looks awesome! I read some of the examples and rationale today, and the new API makes so much sense. Reminds me how some of my own component designs have evolved as I've gotten more experienced using React. Can't wait to use v4!

@jflayhart

This comment has been minimized.

Show comment
Hide comment
@jflayhart

jflayhart Sep 19, 2016

Really would love to see efforts for #2286. I am of the opinion, however, that this is a bug with react-router and we should be able to specify if an :id param is alphanumeric or of type number. It took me some time to debug these routing issues until I found this ticket; only to realize I just have to create a workaround for now.

jflayhart commented Sep 19, 2016

Really would love to see efforts for #2286. I am of the opinion, however, that this is a bug with react-router and we should be able to specify if an :id param is alphanumeric or of type number. It took me some time to debug these routing issues until I found this ticket; only to realize I just have to create a workaround for now.

@johnnyreilly johnnyreilly referenced this issue Sep 27, 2016

Closed

Why react-router/history.d.ts ? #11514

1 of 1 task complete

@mjackson mjackson removed the meta label Sep 28, 2016

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Oct 14, 2016

Collaborator

@jflayhart 4.0 uses path-to-regexp, which lets you do that exact kind of thing.

Are we good to go on 3.0? #3852 is the only thing outstanding, and that can be done in a minor later.

Collaborator

timdorr commented Oct 14, 2016

@jflayhart 4.0 uses path-to-regexp, which lets you do that exact kind of thing.

Are we good to go on 3.0? #3852 is the only thing outstanding, and that can be done in a minor later.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Oct 14, 2016

Contributor

Is 3.0 still going to be released then? I kind of assumed you were going to jump straight to 4. That's not the case?

Contributor

johnnyreilly commented Oct 14, 2016

Is 3.0 still going to be released then? I kind of assumed you were going to jump straight to 4. That's not the case?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Oct 14, 2016

Collaborator

2.x/3.x will be maintained indefinitely, for those that don't want or aren't able to upgrade to 4.0. 3.0 rolls up some deprecation removals and a few improvements/fixes, but is otherwise a fairly boring release 😄

Collaborator

timdorr commented Oct 14, 2016

2.x/3.x will be maintained indefinitely, for those that don't want or aren't able to upgrade to 4.0. 3.0 rolls up some deprecation removals and a few improvements/fixes, but is otherwise a fairly boring release 😄

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Oct 14, 2016

Contributor

That's actually great news. I know the project I'm working on is fairly large / complex and the likelihood of getting a thumbs up to go to 4.0 may well be a long time coming... So thanks 👍 😄

Contributor

johnnyreilly commented Oct 14, 2016

That's actually great news. I know the project I'm working on is fairly large / complex and the likelihood of getting a thumbs up to go to 4.0 may well be a long time coming... So thanks 👍 😄

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Oct 14, 2016

Contributor

I'm not aware of any blockers.

Contributor

taion commented Oct 14, 2016

I'm not aware of any blockers.

@timdorr

This comment has been minimized.

Show comment
Hide comment
Collaborator

timdorr commented Oct 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment