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

getComponent and getComponents should receive router state instead of location #3262

Closed
btipling opened this issue Apr 8, 2016 · 10 comments
Closed

Comments

@btipling
Copy link

btipling commented Apr 8, 2016

Currently getComponents return a location and not a state and I think it should return state instead.
I would really think it would be easier to determine what components to return from getComponents if the params object was available instead of having to capture state in a closure or parse location manually.

@timdorr
Copy link
Member

timdorr commented Apr 8, 2016

I'm all for this as well. I don't think we can deprecate this easily. So, we'll just have to make a hard cut-over on the next major.

@taion taion changed the title getComponents should receive the state object onEnter gets instead of just location getComponent and getComponents should receive router state instead of location Apr 8, 2016
@taion
Copy link
Contributor

taion commented Apr 8, 2016

@timdorr

That's not correct. You just merge the objects and use deprecation wrappers around the location properties.

It's totally doable on a minor update. Probably not 2.1.x, and possibly not 2.2.x either given that we already have deprecations lined up there. I'd say 2.3.x.

@taion
Copy link
Contributor

taion commented Apr 8, 2016

This is somewhat related to #3232 in privileging getComponent over the route-based async hooks, which should generally not be used. I also mentioned this issue to @KyleAMathews after #3215 to allow resolving based on the matched parameters in getComponent, but hadn't realized that it would be possible to do a reasonable deprecation here until a couple of days ago.

@taion
Copy link
Contributor

taion commented Apr 8, 2016

Ref gatsbyjs/gatsby#222.

@timdorr
Copy link
Member

timdorr commented Apr 8, 2016

Whoops, I thought LD's had a params prop for some reason. Yeah, it should be possible to do on a minor then.

@taion
Copy link
Contributor

taion commented Apr 8, 2016

Yup. The main issue is just scheduling. It's going to be much easier for users if we release a deprecation or two every couple weeks or every month. Much easier to keep up than if we drop a half dozen deprecations all at once every few months and effectively require people to rewrite half of their router-related code.

@timdorr
Copy link
Member

timdorr commented Apr 8, 2016

Well, it's death by a thousand cuts vs one fell swoop. Having a BC deprecation path is lightyears ahead of something like Angular 1.x versioning (I have plenty of apps still stuck on 1.2.x for this exact reason), so I don't think grouping up the releases is a bad thing. It's all @ryanflorence and @mjackson's call, since they have publish access and we don't.

@taion
Copy link
Contributor

taion commented Apr 8, 2016

Not to totally derail things, but the experience for users on slow upgrade cycles is the same no matter what.

The benefit of more frequent releases is that I can devote a day or so every two-week sprint to pay down tech debt and upgrade dependencies without derailing things if the releases are staged, but I can't really stop working on my product for half a week or a week if we wait several months between releases and do a big bang.

It's just strictly more optionality, and matches better to how people actually update dependencies.

@taion
Copy link
Contributor

taion commented Apr 8, 2016

FWIW, I think that's what was really the problem with the 1.x and 2.x releases. 1.x required a full rewrite, and 2.x required multiple changes in every place where users touched the router. It's definitely not the right way to go.

Even with deprecations, if I see myself getting stuck with a dozen deprecation warnings in a dozen different parts of my code, I'm much more likely to just postpone the upgrade, possibly indefinitely.

@jquense
Copy link
Contributor

jquense commented Apr 8, 2016

just to echo @taion s tangential point, we have a bunch of stuff mostly stuck on v1.0 of the router because I just haven't had time to take week to upgrade and make sure something unexpected didn't break, and update all the points we extend RR

@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

4 participants