-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
11 changed files
with
208 additions
and
142 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
class ChangeEmitter { | ||
|
||
constructor() { | ||
this.changeListeners = []; | ||
} | ||
|
||
_notifyChange() { | ||
for (var i = 0, len = this.changeListeners.length; i < len; ++i) | ||
this.changeListeners[i].call(this); | ||
} | ||
|
||
addChangeListener(listener) { | ||
this.changeListeners.push(listener); | ||
} | ||
|
||
removeChangeListener(listener) { | ||
this.changeListeners = this.changeListeners.filter(function (li) { | ||
return li !== listener; | ||
}); | ||
} | ||
|
||
} | ||
|
||
export default ChangeEmitter; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
11cdab3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjackson Is this state meant for data coming from the apps or for internal things like scroll position? I'm asking because one of the ideas of
state.key
was so that we don't have to store data inhistory.state
and usesessionStorage.set(location.key, state)
instead, which makes it possible to store state after pop transitions.11cdab3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taurose it's for both.
The idea here is that you can give us some state and we'll store it and give it back to you on
location.state
when you need it. We reserve a few properties for the router, includingkey
,scrollX
, andscrollY
. If you create an instance ofHashHistory
with aqueryKey
, we use a query string parameter +sessionStorage
for persistence.Does that work?
11cdab3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also planning on putting a
state
prop on<Link>
so that links can pass stuff into this state.11cdab3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would, but why not use
sessionStorage
forBrowserHistory
as well? As I said, the drawback I see in using the built in history state is that it can't be changed after a transition, i.e. you couldn't provide an API for setting state incomponentWillUnmount
ortransitionHook
for pop transitions becausereplaceState
would override the state of the next page. I guess that means for use cases like #1188 we'd have to callreplaceState
oncomponentDidUpdate
, or uselocation.key
and manage it ourselves.Setting
state
with<Link>
seems like a good idea. How is this related to #1273 ? I think we should differentiate between persistent state and state that's only used once.11cdab3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we could totally do that. I actually had that same thought. In fact, @ryanflorence just suggested that we might be able to use
componentWillUnmount
to save the scroll state instead of hard-coding it into the DOM history implementations. That would definitely be an interesting exercise. You up for making a PR @taurose? :)I think the only state we need to worry about is the persistent kind. If you have state you only use once, you need to figure out some other way to pass that around.