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

m.prop() resolves promises #197

Merged
merged 4 commits into from
Aug 16, 2014
Merged

m.prop() resolves promises #197

merged 4 commits into from
Aug 16, 2014

Conversation

Zolmeister
Copy link
Contributor

This extends #169

test(function() {
    var defer = m.deferred()
    var prop = m.prop(defer.promise)
    defer.resolve("test")

    return prop() === "test"
})
test(function() {
    var defer = m.deferred()
    var prop = m.prop(defer.promise).then(function () {
        return "test2"
    })
    defer.resolve("test")

    return prop() === "test2"
})

@Zolmeister Zolmeister mentioned this pull request Aug 13, 2014
@lhorie
Copy link
Member

lhorie commented Aug 13, 2014

Maybe I'm just missing something obvious, but the promise itself already does the same thing

var defer = m.deferred()
var prop = defer.promise
defer.resolve("test")
console.log(prop() == "test") // true

What's the use case for re-wrapping it in another m.prop?

@Zolmeister
Copy link
Contributor Author

True, but if someone wants to use their own promise library (like Promiz or Q) this makes it trivial

@pygy
Copy link
Member

pygy commented Aug 13, 2014

You can "propify" a non-Mithril promise like this:

function propify(promise) {
    prop = m.prop()
    promise.then(prop)
    prop.then = _then.bind(null, promise)
    return prop
}

function _then(promise, res, rej){
    return propify(promise.then(res, rej))
}

//Add your own propifier, e.g. for ES 6 promises:

function propifiedPromise(executor) {
    return propify(new Promise(executor))
}

Edit: This may be worth a Wiki entry... Done.

@Zolmeister
Copy link
Contributor Author

@pygy I know, I am suggesting an improved prop() behavior based on a real use-case

@pygy
Copy link
Member

pygy commented Aug 13, 2014

I hadn't looked at your code yet.

Your first patch was invasive, but the current one is an improved version of the snippet above, and it could probably live out of the core.

I'd rather see Mithril stay as lean as possible. I'm currently writing an app that will hopefully fit, gzipped, in the first TCP packet of the response: ~14 KB of HTML, JS, CSS and SVG. Other resources are loaded in separate requests. Each byte counts here :-).

@Zolmeister
Copy link
Contributor Author

@lhorie What is your stance on this? We heavily depend on this framework (https://github.com/claydotio) for our applications, and it would benefit both of us if our contributions were merged upstream.

@lhorie
Copy link
Member

lhorie commented Aug 14, 2014

Ah I see.

I don't like the idea of hacking m.prop to support a m.deferred-related feature, but the ability to swap Mithril's built-in promises w/ 3rd party ones has been requested before enough times that I should probably do it.

I was thinking that it would look something like this:

m.deferred.promise = Promise //expect ES6 implementation/polyfill

Implementation would likely do the above w/ Promiz or Promiscuous (or a fork), and use @pygy's propify tool internally to get propified promises through the m.deferred/m.request APIs.

The main problem though is that conforming to A+ is a breaking change, e.g. we would no longer be able to chain side-effect functions:

var getThings = function() {
  return m.request({method: "GET", url: "/things", background: true}).then(m.redraw)
}

Instead you'd need to start using the more verbose version

var getThings = function() {
  return m.request({method: "GET", url: "/things", background: true})
    .then(function(value) {
      m.redraw()
      return value
    })
}

Anyone have thoughts, opinions, objections on going ahead with this breaking change?

@Zolmeister
Copy link
Contributor Author

Normally, that use case is done with .tap(), so that could be added to the promise if it seems useful

promise.tap(m.redraw)

@lhorie
Copy link
Member

lhorie commented Aug 14, 2014

Is that part of the ES6 API? I was under the impression it only exposed then and catch on the promise instance. I'm gonna need to investigate what is involved in exposing propified non-standard methods for all the major promise libraries. I have a feeling that interop w/ non-standard features is a completely different beast than just interop w/ standard ES6 Promises.

@Zolmeister
Copy link
Contributor Author

No, I was suggesting simply adding it to the Mithril promise implementation as a convenience for developers.

@Zolmeister Zolmeister merged commit d567de0 into MithrilJS:next Aug 16, 2014
@dudeman
Copy link

dudeman commented Aug 27, 2014

Just upgraded to 0.1.21 and definitely miss being able to chain side-effect functions on to promises

promise.then(m.redraw)

any plans to implement something that would allow this?

@Zolmeister Zolmeister deleted the promiseprop branch August 27, 2014 21:44
@Zolmeister
Copy link
Contributor Author

@dudeman you can go ahead and open an issue for adding .tap() on the promise

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.

4 participants