-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
DashRenderer is now a class that can be called with an optional hooks argument
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.
Looks good! Just a few small changes
@chriddyp Thanks for your detailed explanation! Had to write some logic for the case of hooks not being used, hope I got it right. |
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.
The storedHooks
vs hooks
feels a little convoluted to me and it seems like it's mostly just an artifact of our own data flow from config
argument to getting something into the store
.
Reading through this a second time, it seems like getting the data into the store is causing some complexity. Since this data is static (it doesn't change through the lifecycle of the app), I don't think that we really need to wire it into the store nor create our own actions.
Instead, could we just thread hooks
down through to the actions? We're already threading it through the first few components (AppProvider
➡️ AppContainer
➡️ APIController
), so let's just keep threading it through until we get to the place where we're firing the actions: ➡️ TreeContainer
➡️ NotifyObservers
and then wire it in as arguments in the dispatch(notifyObservers())
API calls that the NotifyObservers
component makes. (e.g. here:
dispatch(notifyObservers({id: ownProps.id, props: newProps})); |
If we add more config arguments in the future, then we can abstract out hooks
into a config
argument and pass our data down this way.
The current solution of making these dispatches (and causing rerenders) inside APIController
doesn't seem like it landed quite right, due to the complexity of hooks
vs storedHooks
and the fact that this data is static (it's not asynchronous like the rest of the API actions in APIController
).
Another solution might be to place it into the store when we create it, that is here: dash-renderer/src/AppProvider.react.js Lines 7 to 11 in bd3c8b3
store.initializationConfig = {hooks: hooks} . Or maybe there is another way to do this with the redux reducer initialization sequence. However, that seems a little less straightforward / standard than just wiring everything through.
|
FWIW I agree that wiring these hooks through the code is the way to go. Another approach for "global static config" is to use React |
@bpostlethwaite Yeah, isn't the I'm also wondering: if I thread it all the way down, won't it fire for each component wrapped in a |
Context has been part of the React API for a long time - it's recently been elevated out of "don't use this" status. But still it's probably best not to use it for the fact that it can make code harder to test / reason about. |
Won't that pass the |
@chriddyp Actually, thinking about it again, I don't think the Here's a stack overflow question about it |
No it won't, it will only pass it down to the
I'm not sure what you mean by "fire". It won't cause any extra renders though, because it will just be integrated into the initial props of all of the parents.
Can you verify this yourself by putting in debugger calls?
The main thing that I'm concerned about is that the order of operations needs to be very clear. If we have this done in |
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.
Fantastic @valentijnnieman - I believe we've reached the finish line! 💃
@chriddyp pretty sure all your comments from last fall have been long since addressed - can I dismiss your review? |
👍 💃 ! |
This adds customizable hooks before and after the fetch to
dash-update-component
, and makes thedash-renderer
a standalone class calledDashRenderer
that can be called with an optional object holdingrequest_pre
andrequest_post
functions, a la #65. Here's a simple example:It uses the new customizable
index_string
to setup a customDashRenderer
instance.This depends on
dash
having support for a standalonedash-renderer
, so this PR should be merged along side of this one.