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

Refine microcosm-http. and microcosm-dom Add file-uploader example #509

Merged
merged 3 commits into from
Apr 24, 2018

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Apr 24, 2018

This commit moves the file-upload example from microcosm-http over to the examples directory and updates it with a cleaner interface.

In the process of doing this, I made a few improvements that I've pointed out within the file diff.


uploader

onSend={this.trackProgress}
>
<Progress status={status} file={file} onCancel={this.queue.clear} />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New here: you can pass a Subject to ActionForm and ActionButton. This lets you control the underlying scheduling.

New here: Subject:clear(). This lets you cancel all subscriptions but keep the subject open. It's sort of like a reset.

trackProgress = action => {
action.every(iteration =>
this.setState({ status: iteration.status, file: iteration.payload })
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New here: action.every is a subscription callback that fires on every status change.

@@ -15,7 +15,7 @@ export function generateActionButton(createElement, Component) {
}

componentWillUnmount() {
this.queue.cancel()
this.queue.complete()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to cancel. That would cancel any outstanding actions, which might not be what the user wants. If that is the desired behavior, you can now do that yourself.

@@ -43,21 +43,19 @@ export function generateActionForm(createElement, Component) {

submit(event) {
let result = this.send(this.props.action, this._parameterize())
let action = Subject.hash(result)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping the result in Subject.hash gives me either:

  1. The same action back
  2. A Subject version of whatever send returned.

That means you can pass anything into the send prop and the action form will turn it into the same interface as an action (Subjects). You'll always get a Subject back from send now, every time.

So you don't really need a repo. Just a send method:

let send = () => Promise.resolve(true)
let callback = (subject) => {}

<ActionButton send={send} onSend={callback}/>

Copy link
Contributor

@ipbrennan90 ipbrennan90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool Nate. I like the additional control your adding by including the Subject.cancel() stuff. Excited to use this soon. 👍

import { Progress } from './progress'

function asFormData(form) {
return { data: new FormData(form) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's FormData?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a part of XMLHttpRequest 2.0:

The FormData interface provides a way to easily construct a set of key/value pairs representing form fields and their values, which can then be easily sent using the XMLHttpRequest.send() method. It uses the same format a form would use if the encoding type were set to "multipart/form-data".

https://developer.mozilla.org/en-US/docs/Web/API/FormData

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, is it naive to think that this would be a reasonable default for the serializer ActionForm property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was just typing this up. I'd be okay with dropping form-serialize and just making this the default. I wonder what turning FormData into JSON would look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue filed:
#510

file: undefined
}

queue = new Subject()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is Subject the de facto implementation of an Observable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using it waaay more than Observable. It's a way to get the observable API externally, like this, but inside out:

new Observable(observer => {
  observer.next(1)
  observer.next(2)
  observer.next(3)
  observer.complete()
})

Can be:

let subject = new Subject()

subject.next(1)
subject.next(2)
subject.next(3)
subject.complete()

The advantage of an observable is that the behavior inside is lazy based on if anything subscribes to it. Subjects are more active.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome. Not sure if Subject is already in your crosshairs for "needs a better name", but it's quite vague at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. It comes from Observables literature, but I agree that it's vague. I'd be happy to figure out another name.

Working more on scheduler stuff, I have some times wondered about calling this Job or Process. But I dunno.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue filed:
#511

@@ -168,6 +168,30 @@ describe('Subject', function() {
})
})

describe('clear', function() {
it('resets the usbject', function() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: subject

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0e03b9c

@@ -540,7 +540,7 @@ describe('::teardown', function() {
expect(renders).toBe(1)
})

it('changes during teardown do not cause a recalculation', function() {
it('changes during teardown do not cause a recalculation', async function() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total nitpick, but this might read better as:

it('does not recalculate when changes during teardown occur'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0e03b9c

@@ -168,6 +168,30 @@ describe('Subject', function() {
})
})

describe('clear', function() {
it('resets the usbject', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usbject

Copy link

@zporter zporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks awesome, @nhunzaker!

@efatsi
Copy link
Contributor

efatsi commented Apr 24, 2018

Cool stuff, like the way that click and submit have simplified down.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@afbf1cb). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #509   +/-   ##
========================================
  Coverage          ?   99.7%           
========================================
  Files             ?      27           
  Lines             ?     672           
  Branches          ?     133           
========================================
  Hits              ?     670           
  Misses            ?       2           
  Partials          ?       0
Impacted Files Coverage Δ
packages/microcosm-dom/src/action-button.js 100% <100%> (ø)
packages/microcosm/src/history.js 100% <100%> (ø)
packages/microcosm-dom/src/presenter.js 100% <100%> (ø)
packages/microcosm-dom/src/action-form.js 100% <100%> (ø)
packages/microcosm/src/subject.js 100% <100%> (ø)
packages/microcosm-http/src/http.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afbf1cb...d72a0ef. Read the comment docs.

@nhunzaker nhunzaker merged commit f9b316f into master Apr 24, 2018
@nhunzaker
Copy link
Contributor Author

Thanks, guys!

@nhunzaker nhunzaker deleted the microcosm-dom-updates branch April 26, 2018 21:09
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.

5 participants