Uh oh!
There was an error while loading. Please reload this page.
Hot reload - #73
Conversation
T4rk1n
commented
Sep 11, 2018
@plotly/dash Ready for review! |
rmarren1
left a comment
There was a problem hiding this comment.
Nice! There's two questions I had, and two newlines missing
| }), | ||
| dispatch => ({dispatch}) | ||
| )(Reloader); | ||
No newline at end of file |
Uh oh!
There was an error while loading. Please reload this page.
| if (!disabled && !this._intervalId) { | ||
| this._intervalId = setInterval(() => { | ||
| if (!this.state.reloading) { | ||
| dispatch(getReloadHash()); |
There was a problem hiding this comment.
Should state.reloading be set to true here? I do not see anywhere that state.reloading is changed, it seems to always be false.
There was a problem hiding this comment.
I think it's a leftover from something I tried, I will remove thanks.
| function rootReducer(reducer) { | ||
| return function(state, action) { | ||
| if (action.type === 'RELOAD') { | ||
| const {history} = state; |
There was a problem hiding this comment.
I there extra stuff in state that is stripped when you do this? I don't see what these two lines do, I don't think state is changed here
There was a problem hiding this comment.
Everything but the history is stripped down, it triggers a soft reload.
There was a problem hiding this comment.
Oh okay so everything else gets set to default except the history, looks good
There was a problem hiding this comment.
Hmm, can you not combine this reducer into the recordHistory function that's already there? I'm not sure I understand why it is a separate reducer.
There was a problem hiding this comment.
I think it should be separate from recordHistory. This reducer doesn't really change the history, it just removes everything else. It could have a different name though for sake of clarity, maybe like preserveHistoryOnReload
valentijnnieman
left a comment
There was a problem hiding this comment.
Left you a couple of questions!
| function rootReducer(reducer) { | ||
| return function(state, action) { | ||
| if (action.type === 'RELOAD') { | ||
| const {history} = state; |
There was a problem hiding this comment.
Hmm, can you not combine this reducer into the recordHistory function that's already there? I'm not sure I understand why it is a separate reducer.
| } | ||
| if (reloadHash.content.reloadHash !== this.state.hash) { | ||
| // eslint-disable-next-line no-undef | ||
| window.clearInterval(this._intervalId); |
There was a problem hiding this comment.
So the no-undef rule here should not trigger if it's sure that this._intervalId is set, right? Maybe it's better to write a check here that makes sure that it's not null, instead of disabling the rule.
There was a problem hiding this comment.
The no-undef is for window, it highlight as error in pycharm.
There was a problem hiding this comment.
The rootReducer is for soft-reload, it destroy the state of the store and then the other reducers can reload the application. I think it's cleaner to keep it separate as the logic for the historyReducer come after and they do different operations.
There was a problem hiding this comment.
Ah ok! Hmm maybe that rule can be turned off globally then, if it's always going to trigger on window?
There was a problem hiding this comment.
Last question: isn't it maybe better to put the rootReducer in a separate file, and call it something else, like reloadReducer or whatever?
There was a problem hiding this comment.
maybe that rule can be turned off globally then
I think we can put window in a global exclude or something. Anyway it underline as error in IDE, I just hit alt-enter and select a fix, doesn't take more than a second.
Last question: isn't it maybe better to put the rootReducer in a separate file
It's only a couple lines and is the last reducer exported, keeping it in reducer make more sense to me.
| if (reloadHash.content.hard) { | ||
| // Assets file have changed, need to reload them. | ||
| // eslint-disable-next-line no-undef | ||
| window.top.location.reload(); |
There was a problem hiding this comment.
Hmm, why is the no-undef rule triggering here?
| dependenciesRequest: API.dependenciesRequest, | ||
| layoutRequest: API.layoutRequest, | ||
| loginRequest: API.loginRequest, | ||
| reloadHash: API.reloadRequest, |
There was a problem hiding this comment.
Any reason why it's named reloadHash here and reloadRequest on API.reloadRequest?
There was a problem hiding this comment.
Good catch, I refactored reloadHash to reloadRequest to conform to the other API, I guess that one didn't follow up.
| disabled: true | ||
| } | ||
| } | ||
| this._intervalId = null; |
There was a problem hiding this comment.
Can _intervalId not be in state as well?
| hash: null, | ||
| interval, | ||
| disabled: false | ||
| } |
There was a problem hiding this comment.
What if this state is also handled in a reducer? Would there be value in having this state in the store? Perhaps saving the hash in the store for example would make it easier to at some point in the future to enable the store to be saved client-side, so that it can be rehydrated upon refreshing.
There was a problem hiding this comment.
The hash is null at first, it is set in the state from the server on the first hash request so I don't think it needs to be rehydrated.
9c9c797 to
232b6a4Comparermarren1
left a comment
There was a problem hiding this comment.
Just a few minor style changes 💃
| *.csv | ||
| .idea/ | ||
| .vscode |
| disabled: false, | ||
| intervalId: null, | ||
| packages: null, | ||
| max_retry: max_retry, |
| ); | ||
| let node = it.iterateNext(); | ||
| while (node) { |
There was a problem hiding this comment.
Is node a proper iterator? You should be able to do const nodesToDisable = [...it.iterateNext()]
There was a problem hiding this comment.
It's a proper iterator but it doesn't work, if there's no element found (asset file added) it will fail to make an array with the iterator.
| nodesToDisable.push(node); | ||
| node = it.iterateNext(); | ||
| } | ||
| nodesToDisable.forEach(n => |
There was a problem hiding this comment.
I know we are trying to use ramda 🐏 in this repo even when a core library function would do the job. I think R.forEach would do the job.
| } else if (reloadRequest.status === 500) { | ||
| if (this._retry > this.state.max_retry) { | ||
| window.clearInterval(this.state.intervalId); | ||
| // Integrate with dev tools ui?! |
| function rootReducer(reducer) { | ||
| return function(state, action) { | ||
| if (action.type === 'RELOAD') { | ||
| const {history} = state; |
There was a problem hiding this comment.
I think it should be separate from recordHistory. This reducer doesn't really change the history, it just removes everything else. It could have a different name though for sake of clarity, maybe like preserveHistoryOnReload
Available with the following version:
dash==0.27.0rc7dash-renderer==0.14.0rc6Add hot-reload capability on project file changes by periodically requesting a hash and checking it's the same hash as the initial one.
Adds:
Need plotly/dash#362