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

Explicitly take path instead of location for match #2676

Closed
wants to merge 1 commit into from
Closed

Explicitly take path instead of location for match #2676

wants to merge 1 commit into from

Conversation

taion
Copy link
Contributor

@taion taion commented Dec 8, 2015

I updated the docs here, but not CHANGES because we need to comprehensively update them ahead of an interim release.

  • API docs on this were out of date.
  • It's almost never useful to try to call createLocation yourself to make a location object; it's almost always less buggy and easier to just pass in the path.
  • This is forward-compatible with e.g. updating the history.createLocation API to take a location descriptor (rather than a list of args). (but I don't think there's a good reason to encourage users to pass in a location descriptor here)

@@ -230,7 +230,7 @@ A *router* is a [`history`](http://rackt.github.io/history) object (akin to `win
There are two primary interfaces for computing a router's next [state](#routerstate):

- `history.listen` is to be used in stateful environments (such as web browsers) that need to update the UI over a period of time. This method immediately invokes its `listener` argument once and returns a function that must be called to stop listening for changes
- `history.match` is a pure asynchronous function that does not update the history's internal state. This makes it ideal for server-side environments where many requests must be handled concurrently
- `history.match` is a function that does not update the history's internal state. This makes it ideal for server-side environments where many requests must be handled concurrently
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just thrown in; that function is definitely not pure async.

@taion
Copy link
Contributor Author

taion commented Dec 8, 2015

Gah this is probably going to have some (trivial) merge conflicts with #2674. Please merge that PR first if we want to go ahead with making an interim release for just updating history API usage.

@taion
Copy link
Contributor Author

taion commented Dec 8, 2015

My confidence that this is correct is down to 75% or so.

Looking at that API, and especially with the test case I have, it can take a location descriptor, so there's not necessarily a reason to change it (although we'd still need to call history.createLocation or at least parsePath to convert a path string into a location descriptor object).

But I don't think there's any reason to pass anything other than a path string into match.

I'd be okay either way as long as we kept the fixes to the docs for match in API.md.

@taion taion added this to the v1.1 milestone Dec 8, 2015
@timdorr timdorr mentioned this pull request Dec 8, 2015
4 tasks
@timdorr
Copy link
Member

timdorr commented Dec 8, 2015

Looks like we're getting around to router's part of #2186 😄 (now that history has done its part)

This could start getting out of hand with changing up matchRoutes to accept LDs and strings (it just accepts LDs/locations right now).

We don't have any sort of parsePath locally, which is why I was wondering why we're getting rid of createLocation in history. It makes implementing #2186 relatively easy on router's side. Also, we don't have to worry about divergent implementations of the same functionality. It would seem to me that makes this PR a lot more clear if it continues to exist as a history API.

@taion
Copy link
Contributor Author

taion commented Dec 8, 2015

matchRoutes should only accept a location object IMO - I don't think we should have any APIs that are "location or location descriptor"... I think.

@taion
Copy link
Contributor Author

taion commented Dec 8, 2015

What do we think? Is there any reason to accept anything other than a path string for match?

@timdorr
Copy link
Member

timdorr commented Dec 8, 2015

Yes, for state. It's currently being done by users, so we need to support both strings and descriptors for this API.

@taion
Copy link
Contributor Author

taion commented Dec 8, 2015

Fair enough. Closing this for now, then.

@taion taion closed this Dec 8, 2015
@taion taion deleted the update-match-path branch December 8, 2015 22:39
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 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

Successfully merging this pull request may close these issues.

2 participants