-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
Thanks @lucasconstantino - very nicely done! I'm sure this will be a useful addition. Let me review the code in detail, but I can see we'll need to get tests to pass (looks like the new build files need to be included in |
const {dispatch} = this.props; | ||
dispatch(readConfig()); | ||
const {dispatch, initialConfig} = this.props; | ||
dispatch(readConfig({config: initialConfig})); |
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.
Let's move || JSON.parse(document.getElementById('_dash-config').textContent)
up here. We never should have put that in the reducer in the first place, but with the extra logic it's even more out of place.
Also the extra nesting is unnecessary, can't we just do readConfig(initialConfig || ...)
?
@@ -19,57 +19,63 @@ import { | |||
omit, | |||
pick, | |||
propOr, | |||
type |
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.
Oh interesting - seems the pattern we have in our prettier
calls in the package.json
miss the top level files in src/
- would you mind changing
src/**/*.js src/**/*.react.js
to
src/*.js src/**/*.js
so we know this formatting is consistent with the rest of our codebase?
(the *.react.js
variant also doesn't seem to be necessary, *.js
covers it)
_dashprivate_config: stateProps.config, | ||
}) | ||
(stateProps, dispatchProps, ownProps) => { | ||
const registry = ownProps._dashprivate_registry || Registry; |
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.
This will work, but perhaps it would be simpler to merge registry
into config
(in AppContainer
per #173 (comment), including this fallback to Registry
) which puts it in the store
?
@@ -29,13 +29,16 @@ AppProvider.propTypes = { | |||
registry: PropTypes.shape({ | |||
resolve: PropTypes.func, | |||
}), | |||
name: PropTypes.string, | |||
enforceNew: PropTypes.bool, |
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.
Not crazy about the name enforceNew
- newStore
or localStore
perhaps? Or could we just use name
- if you supply a name
we make a new store, if not we use the global one?
This makes me wonder what happens if the wider app remounts a dash app component. Could we perhaps cache the store based on name
so the dash app wouldn't need to reload completely when it's remounted? That might have other issues, dunno, but if we think that's a goal to be working toward, we may want to prepare for it by telling people to use a unique name
for each dash app on the page.
And... then if we do that (I'm getting a few steps ahead of myself here, I really am curious what you think of all the above and am happy to have a discussion about it, but in case you agree I'll keep going) perhaps name
is a little less descriptive than we might like - maybe change it to appKey
or dashAppKey
?
@lucasconstantino FYI we're merging |
7ecef9a
to
29cb3d1
Compare
The problem
Working as a front-end developer, I find using Dash to be quite unproductive. Front-end JavaScript evolves fast, and I feel like a framework like Dash will either be left behind in terms of this evolution, or will become too opinionated, ending with a very specific stack, making adoption harder.
The solution
I think it would be very beneficial to this project to move forward into a (optionally) decoupled solution, which at some point would allow for the focus to be put on the good parts which are hard to handle outside the framework. This would mean this project would then be fit for integration in any other React based application - Next.js, Gatsby, Create React App, or whatever the cool folks put on the table next.
How
This PR introduces the capability to use the
dash-renderer
sub-project in a completely decoupled way. To make it possible, we had to change a couple of global-based parts of the renderer:window[namespace]
availability;Usage
Once accepted, this PR would allow other applications to directly use
AppProvider
, which was updated to provide an API for outside control of the parts mentioned above. An example of usage could be to have a React component receive apath
, and then useAppProvider
to render whatever Dash provides on thatpath
:...which could then be used like that: