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

URLStore should be refactored and pluggable #166

Closed
seanadkinson opened this issue Aug 4, 2014 · 6 comments
Closed

URLStore should be refactored and pluggable #166

seanadkinson opened this issue Aug 4, 2014 · 6 comments

Comments

@seanadkinson
Copy link
Contributor

The URLStore is getting a little messy, with essentially switch statements in most of the methods, with functionality changing based on value of _location. This lends itself nicely to a refactor with separate implementations for each type. This would also allow for other implementations, such as an implementation that supports History.js.

@ryanflorence
Copy link
Member

yessir!

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

👍

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

@seanadkinson Maybe we could reuse the <Routes location> prop.

<Routes location="hash"/> <!-- sugar for <Routes location={HashLocation}/> -->
<Routes location="history"/> <!-- sugar for <Routes location={HistoryLocation}/> -->
<Routes location={HistoryJsLocation}/>

What do you think?

@sophiebits
Copy link
Contributor

I like that.

@seanadkinson
Copy link
Contributor Author

Here's first pass: odysseyscience@bef9e19

This will add Router.locationsHandlers map, which can be added to by external code. The Routes.location prop just looks here for a mapped location handler. Wasn't sure if there was a more common way to expose mutable configuration for the world to plug in to.

Still need to look closer at the tests. One was failing that said you shouldn't be able to go back before the beginning of history, but according to MDN it isn't supposed to throw an error anyway, so that test may be invalid.

This "location handler" API will be really easy to plug History.js into, something like:

Router.locationHandlers.historyjs = require('HistoryJsLocationHandler')

And

<Router.Routes location="historyjs">

The refactor also made it easy to see some legacy code in URLStore that was intending to manage history in memory - perhaps just for testing?

Initial thoughts or suggestions?

@ryanflorence
Copy link
Member

closing, follow #169

seanadkinson added a commit to odysseyscience/react-router that referenced this issue Aug 6, 2014
mjackson pushed a commit that referenced this issue Aug 13, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 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 a pull request may close this issue.

4 participants