-
Notifications
You must be signed in to change notification settings - Fork 60
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
docs($readme): add Complete Example using history
package
#118
Conversation
Does anybody use history v4 independently, except via RRv4, though? |
I use it for pure-redux-router (will likely be renamed to That said, I think the If you don't want it in the readme, perhaps link to a second doc with this example so those interested will find it. |
The issue is that, if you're really trying to do routing based on history, following something like the example you have is not really a great idea, because you don't get the right guarantees over when the scroll update happens, relative to the rerender, without making specific order-dependent assumptions. I think maybe a better way to go would be to add links to https://github.com/taion/react-router-scroll and https://github.com/4Catalyzer/found-scroll. As a historical note, this library did use to be a history integration, but that turned out to just not be a great idea, because it didn't quite work right. |
Did I get the "order-dependent-assumptions" right in the example? Basically it seems the issue you're talking about is the one regarding guaranteeing the hook fires before history listen handlers on pop state events? If that's the case, you're saying you have a useless package unless it's used by another package which itself is closely coupled enough to the rendering mechanism, OR you simply document it like I've done (and I should add the notes about the required order) so that other render-coupled packages know what to do. A lot of packages make specific requirements about the order of things in user-land--sometimes there is no way around it (such as the packages that buffer CSS, and require you empty the buffer immediately after calling But quick question: the hook firing before re-renders is only an issue on pop state (browser button usage), not--as described in my example--where you trigger renders (perhaps in response to redux actions) and then manually call |
Also the links to those packages are now irrelevant since everybody is using the latest React Router. How are new packages supposed to be created that make use of this without proper directions? Did the fork get it right? What enabled it to get it right? This line: https://github.com/ytase/react-router-scroll/blob/master/src/ScrollBehaviorContext.js#L53 So my example should simply represent how to get that right. |
In the context of React Router v4, putting the update in In general, I don't see the value over including an example here that isn't actually seeing use and may not cover all the edge cases, versus just directing people to one of the existing packages as an example to follow. |
And, yes, from what I've seen, it's best to not use this package directly, and instead use an integration with a router that covers all the edge cases as best as possible. Like, I moved the |
I'm personally building an integration for a new router. A lot of new routers use the If helping developers forge new paths forward is of no interest to you, fine, there's nothing I can say. But React Router isn't the be all, end all. So I've analyzed everything in depth. I think that there is still discrepancies of how to get this right speaks to itself. So basically I'm willing to get this right for a new router, and document the main characteristics back to your repo so they are clear for anyone else, which they are not. That said I was more clear about it before we talked, now I'm worried about the implementation I'm using which I documented: const scrollBehavior = new ScrollBehavior({
addTransitionHook: history.listen,
...
})
history.listen(location => {
const path = location.pathname
// usually in systems that listen to address bar changes, the address bar
// also changes in response to actions which already handle rendering (e.g.
// you manually call `history.push(newPath)`), and as a result shouldn't
// trigger rendering again. Therefore the following rendering is only
// triggered in response to pop state events, i.e. where the browser
// back/forward buttons were pressed:
if (path !== alreadyHandledPathname) {
alreadyHandledPathname = path // prevents manual rendering mechanism
// DO SOME RENDERING!!
// e.g: store.dispatch({ type: 'POP_STATE', payload: { path }})
}
scrollBehavior.updateScroll(prevContext, location)
prevContext = location
}) That And then in the handler passed to My understanding--and perhaps this is a pre-React 16 thing only-- is that dispatching on the store would synchronously lead to the updating of all the components before |
or as I've also been saying, is it a problem because rendering that calls |
I've checked, and here in response to browser button usage: if (path !== alreadyHandledPathname) {
alreadyHandledPathname = path // prevents manual rendering mechanism
// do some rendering
// e.g: store.dispatch({ type: 'POP_STATE', payload: { path }})
}
scrollBehavior.updateScroll(prevContext, location) components will render completely and In short, React (pre React 16) will synchronously render and re-mount everything in response to a In addition, I'm using The only thing that could be wrong is that Anyway, I understand you have limited time. I just thought we could help each other by making some of these problems less abstract. I still don't fully get it. My implementation is working perfectly in modern browsers. But that's not the point--it's older browsers these specifics matter most for. I'm more than willing to give back if you can help me delineate with granular detail the order-based assumptions etc. |
If you're integrating with React, the right place is in Even if you're dispatching a Redux action (e.g. even with Found and React Navigation that both put their routing state inside the Redux store), you still presumably have some sort of router component that handles the actual rendering – and this component should be the one triggering the scroll updates. Anyway, my experience working with this library suggests that the target consumer here is a library developer working on a router integration, not on an application developer – this seems to be your use case as well. Given that, I do think the existing implementations of integrations on top of this are the best documentation, and that the pattern of putting the scroll update in |
actually the premise of my router is: "everything is state, not components." There are intentionally no routing components beside links. The user is supposed to think in redux state and nothing else. That's perhaps the missing piece in our communication. It maps urls to actions to state. And bi-directionally maps actions to urls. I hear you on the Anyway, look forward to a new example implementation. I'll paste you the repo when it's done. |
I don't see how you'd idiomatically guarantee that in a way that's future-proof with Fiber. |
We'll have to cross that bridge when we get to it. Perhaps some new opportunities will unveil themselves by the time its released. Similar to |
I mean, I can't stop you from using this library however you wish, but that's why putting something in This is more a question of router architecture anyway, but note that even without Fiber, if you want to handle something like GitHub or Twitter's async handling where the page displays a loading indicator while data for the next routes are loading, then you still need something extra anyway. |
namely you want |
No – or at least that's not how I handle it. Essentially the router needs to (and probably should) own data fetching anyway, and then it's a bit of extra bookkeeping. You can walk through how it works in e.g. the Found At any rate there are enough concepts of lifecycle across things like rendering and even core routing interactions in more general cases that, again, |
I'm looking at it and I see it has async component retrieval (which as I saw earlier is a placeholder for real code-splitting--nice!): https://github.com/4Catalyzer/found/blob/master/examples/global-pending/src/index.js ...I'm not sure what you said "NO" to. Im guessing you mean to updating scroll on every single componentDidUpdate or every single dimensions change, but rather on only the important ones, such as new data or components fetched. The goal is to typically wait for the height of the page to be what it would be after all the data has been fetched and the route complete changed and rendered, correct? What do you think about exposing an |
That is the API here. Anyway, the semantics for the right time to update the scroll is a function of the routing framework. Again, try clicking around on GitHub; on GH, the scroll position (and the page re-render) don't happen until the async data requests resolve. This is complicated enough that, in general, even when using history, tying scroll behavior into |
I'm beyond that now. I'm trying to determine whether also providing an updateScroll API to users would be a good fit. On top of putting after places within the framework where data fetching occurs. Basically it seems there is no sure fire place to put it in the framwork besides a place that updates way too often. That's why putting it in th user's hands seems the best fit. Also note: the reason I don't like a context component is because it is updating all the time. Even if short circuited on the virtual dom. Jank is a reality in browsers when it comes to animations. Updates cause the jank. My library is about avoiding all these components whose update patterns are both plenty and hidden to the user, and leaving the sole update control mechanism being redux's connect method + shouldComponentUpdate. |
It's really not that hard. Again, both react-router-scroll and found-scroll handle this correctly, relative to the capacities of the respective routers they integrate with. Are you going to update this PR to point to the existing blessed examples, or not? If not, I'm going to close this, because I think the example you have here carries enough caveats as to not be useful. |
can u answer my question for once bro. U evaded and spoke abstractly the whole way through this conversation FYI. Blessed? U mean found. |
The answer is the exact same as it's always been. Put the logic in |
but it's not. it also must be called after data fetching as well. the location may have already changed and not change again when the data gets there. in addition that component is not acceptable to me because your top level component is constantly updating in response to every location change, even if just in the virtual dom. It's not nit-picking because animation jank is unavoidable in the browser (lots of work must constantly be done to achieve silky smooth animations), and I refuse to let components just update all the time unnecessarily. In sum, all these cycles add up. That context component is a no go for me. Besides the fact that my library is component-free, I've been looking for other ways. Exposing a manual API is a different answer--I'm still not sure what you think about. You have your mind set and don't seem very willing to entertain other possibilities. It doesn't matter anymore--at this point I do grasp the concrete characteristics which are in no way made clear anywhere in this documentation. What is concrete in your head, you describe abstractly and is hardly concrete for anyone else. They need full on examples. You evaded confirming any concrete example I gave in this conversation. In addition, my readme example attempted to explain the characteristics. Your way--linking to the other repos--just does it for them, handling all the assumptions with very little explanation. What else can I tell you. You're absolutely convinced that's the only way, so you refuse to entertain other possibilities. Either I'm an idiot or u could do more to help make things concrete for people. I'll put in the links for you. |
basically only u understand the deficiencies you solved. If others could, perhaps they could find other interfaces into them. The exact characteristics of those deficiencies could be better cataloged. It's "not really that hard," but that's only if you don't want a thorough understanding of everything. |
I don't think this PR is going in the direction I want it to go. I'm going to add a more limited list of links to examples that I know work to my satisfaction. As you've observed, this library only offers an imperative API for updating the scroll position. It's up to whoever's writing the integration to decide when to update the scroll position. For something like React Router, that doesn't integrally handle data loading, it's tricky to get this to work nicely with async use cases. For something like Found, which integrates data loading into the router, we can just wait until all the data have been loaded, and dispatch the scroll update exactly once in that case. If you have specific questions on what to do, feel free to open a separate issue. |
It seems this package and the popular
history
package are perfect companions. Therefore I chose it for the complete example.One thing to note:
addTransitionHook
should be called "immediately before a transition updates the page" in order to handle this browser deficiency:https://github.com/taion/scroll-behavior/blob/master/src/index.js#L102
Yet, the way the
history
package works is that it pushes on the global history state object:https://github.com/ReactTraining/history/blob/master/modules/createBrowserHistory.js#L172
before it notifies listeners:
https://github.com/ReactTraining/history/blob/master/modules/createBrowserHistory.js#L183
So as a result
addTransitionHook
will be called after the address bar has updated. This is obviously a problem with the recent fork for React Router 4 too. Perhaps it's even a problem withrouter.listenBefore
used in the original react-router-scroll if that simply is a wrapper forhistory.listen
.Now that said, perhaps all that matters is that the hook is called before the page rendering, not the address bar changing. If that's the case, in my example by having
history.listen
passed toscrollBehavior
and presumably called first, thehistory.listen
callback setup at the end of the example, which possibly re-renders the app, should fire after the hook gets a chance to look at it, and before a render that possibly updates the page. Does that satisfy the concerns here?...I tried to reproduce the issue in Chrome and I don't see the issue (I presume it's because it onlly occurs in older crappier browsers), so I can't easily reproduce it and guarantee that my example addresses it.
The
history
package does not in fact have anaction
key on itslocation
objects. It's maintained on thehistory
object itself. Let me know what you think about how I addressed that and if it appears the most idiomatic. Ideally only one parameter such ashistory
is passed, andscroll-behavior
looks into it to grab both thelocation
andaction
, but obviously this package is already in circulation and that likely is an impossibility. Whatlocation
objects was this originally created for that hadaction
keys? The standard browser one has that?? I wonder why thehistory
package moved it, or perhaps there's something I'm missing.Final thing: I noticed in the
react-router-scroll
PR there was some talk about how usingsessionStorage
in combination with redirects (embedded within renders) may cause some problems. I honestly didn't fully understand the issue. It seems it has something to do with the fact that if a page is rendered--even if for just a few milliseconds--before it's redirected from,scroll-behavior
will do some of its magic to handle browser deficiencies on the wrong page and might do something janky. Basically,scroll-behavior
expects to do its work on the redirected page. I dunno, but I'm interested in getting to the bottom of it as I'm implementing scroll-restoration usingscroll-behavior
for my redux-first routing package: https://github.com/faceyspacey/pure-redux-router .