Skip to content
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

Add support for Redux DevTools via a plugin #373

Merged
merged 5 commits into from Oct 2, 2017
Merged

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Sep 30, 2017

  • Sends state updates to DevTools; Subscribes to DevTools and sets Uppy state to state provided by DevTools for time traveling; supports replaying all actions, step by step, persisting state, jumping to specific state
  • Works without modifying everything else, which is cool. Though I’m not saying adopting Redux would be too hard (see Redux initial work [proof of concept] #338), and it might provide better structure, but we are just not sure yet;
  • We could add support for custom action type names, and then we’ll get actions like UPPY_ADD_FILE and such (see this comment https://news.ycombinator.com/item?id=15342850);
  • Implemented as a separate plugin

(Try by installing and activating: https://chrome.google.com/webstore/detail/redux-devtools/lmhkpmbekcpmknklioeibfkpmmfibljd?hl=en)

And the second commit is useful regardless:

Don’t store function references in state to keep state serializable
Instead of storing methods in state, find plugin and add methods to the object that we pass to render, when it’s needed.

This might be useful for parsing/stringifying JSON, so we don’t forget: https://www.npmjs.com/package/jsan

screen shot 2017-09-30 at 12 19 13 am

done P.S. This could probably be a plugin 🤔

- sends state updates to devtools
- sets uppy state to state provided by devtools for time traveling
- we could add support for custom action type names, and then we’ll get actions like `UPPY_ADD_FILE` and such

try by installing and activating: https://chrome.google.com/webstore/detail/redux-devtools/lmhkpmbekcpmknklioeibfkpmmfibljd?hl=en
Instead of storing methods in state, find plugin and add methods to the object that we pass to `render`, when it’s needed.
@arturi arturi changed the title Add support for Redux DevTools Add support for Redux DevTools via a plugin Sep 30, 2017
@goto-bus-stop
Copy link
Contributor

this is neat!

Implemented as a separate plugin

🎉

Don’t store function references in state to keep state serializable

AwsS3 also stores functions in state to configure the XHRUpload plugin. That one's a bit harder to avoid I think :(

@arturi arturi merged commit 7c0cc62 into master Oct 2, 2017
@arturi arturi deleted the feature/redux-dev-tools branch October 2, 2017 21:23
@kvz
Copy link
Member

kvz commented Oct 3, 2017

AwsS3 also stores functions in state to configure the XHRUpload plugin. That one's a bit harder to avoid I think :(

Would it help if we'd AwsS3 extends XHRUpload? It always felt slightly off to me that we make people .use two plugins and then they temper with each other (like, what if you have another plugin that wants to temper with XHRUpload). It could be that it's just me, with my OOP hammer in hand everything looks like a nail, and maybe there are good reasons not to do it?

@arturi
Copy link
Contributor Author

arturi commented Oct 3, 2017

@kvz I have no good reasons from experience, but I’ve heard warnings that double-extending is a path to madness. We could do what Dashboard does and .use() internally:

if (!this.opts.disableStatusBar) {
this.core.use(StatusBar, {
target: this
})
}
if (!this.opts.disableInformer) {
this.core.use(Informer, {
target: this
})
}

But Renee @goto-bus-stop would know more, cause he designed the flow, and there might be other parts, like encoding plugins, involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants