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

Promises #1

Closed
benjamingr opened this issue Mar 18, 2014 · 17 comments
Closed

Promises #1

benjamingr opened this issue Mar 18, 2014 · 17 comments
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@benjamingr
Copy link

You provide a basise deferred implementation in mithril.

However, it is quite erm... bad.

Not only it uses the old deferred API in favor of the ES6 promise contructor - it doesn't provide type safety at all which is a major part of promises.

http://jsfiddle.net/3na87/

(correct behavior btw http://jsfiddle.net/yN6wF/ works in Chrome and browsers that support promises, alternatively you can include a promise library)

Please consider reading some more about promises and perhaps improving your implementation. Please reassure me this is not just cargo culting.

@domenic
Copy link

domenic commented Mar 18, 2014

Indeed, you probably want to implement Promises/A+, such that your implementation passes the compliance test suite.

@lhorie
Copy link
Member

lhorie commented Mar 18, 2014

Thanks for the bug report. My implementation is currently pretty naive. I'm not sure I'll be able to use the ES6 API since I return thennables that are also m.prop getter-setters, and I need to read up a bit more on handling system exceptions (e.g. errors like "foo.bar.baz is undefined on line x" oughta show in the console), vs app exceptions (e.g. throw new Error("user not found"), show a message to the user).

@matthewp
Copy link

You should just use promiscuous, tiny and A+ compliant

@Raynos
Copy link

Raynos commented Mar 18, 2014

It's recommended you use bluebird ( https://github.com/petkaantonov/bluebird ) or es6-promise ( https://github.com/jakearchibald/es6-promise )

The former for performance & error handling, the latter for ES6 compliance.

@hiddentao
Copy link

+1 for bluebird, as this keeps in line with Mithril's existing performance goals.

@pygy
Copy link
Member

pygy commented Mar 19, 2014

@lhorie: [...] since I return thennables that are also m.prop getter-setters [...]

What's the use case for these?

@matthewp: It's recommended you use bluebird...

The Bluebird core is 13178 bytes when uglified and gzipped. That's about the size of Mithril before any size reduction.

Promiscuous, without its all method (not necessary for A+ compliance), is 1300 bytes after minification and compression, including the MIT license.

@hiddentao
Copy link

@pygy Good point regarding bluebird size - perhaps the built-in implementation can be used as the fallback unless a more thorough library (such as bluebird) is available.

@Raynos
Copy link

Raynos commented Mar 19, 2014

@pygy performance, correctness, kilobytes. Pick two.

@lhorie
Copy link
Member

lhorie commented Mar 19, 2014

What's the use case for these?

@pygy it's used for the assignment syntax for m.request, e.g.

var users = m.request({method: "GET", url: "/users"})

This lets people create terser, more readable controllers, e.g.

var ctrl = function() {
  this.users = m.request(...);
}

instead of

var ctrl = function() {
  this.users = m.prop([]);
  m.request(...).then(this.users);
}

Regarding A+ compliance, I'm not sure I see the benefits of adding a big implementation into core (and for that matter, what the benefits of compliance are). As I understand, if you're an app developer doing hardcore promise stuff, you can "fix" non-compliant thennables by passing it into one of the existing libraries. I've documented all the use cases that I'm expecting from application developers in terms of thennable usage, but if someone could give me a gist showing me other real-life-ish cases that should be taken into consideration and that I'm not accounting for, I'd appreciate the insight.

My biggest concern right now is that I don't want to swallow exceptions that a developer would expect to see on the console (stuff like null ref exceptions, etc). The throw new Error("show message to user") case sounds legit but I'm not sure how to differentiate the two.

@Raynos
Copy link

Raynos commented Mar 19, 2014

@lhorie promises A+ have very specific rules about error handling that when followed allow writing throw safe code.

If a user forgets to wrap a fake mithril promise in a real promise and does

realPromise.then(function (v) {
  return fakeMithrilPromise()
})

then it breaks the ability to write throw safe code in a subtle fashion.

Some promise libraries (like I think bluebird cc @petkaantonov @spion) will see that a thenable is returned and it's not a real promise and will cast it into a real promise to ensure that the ability to write throw safe code is upheld.

Another obvious problem is people seeing a mithril promise and thinking "i dont need to read the docs, its a real promise I know how those work" and then wasting hours debugging their code because your promises are subtly different.

@WickyNilliams
Copy link

Another obvious problem is people seeing a mithril promise and thinking "i dont need to read the docs, its a real promise I know how those work" and then wasting hours debugging their code because your promises are subtly different.

For me, this is the biggest point. Principle of least surprise is not to be underestimated.

@pygy
Copy link
Member

pygy commented Mar 19, 2014

If you chose to wrap another lib, I think that this would preserve the getter-setter nature of thenables:

promise = new Promise(function(resolve, reject){...})
prop = m.prop()
prop.then = promise.then.bind(promise)
prop.then(prop)

If you call your thenables "thenables", rather than promises, it will dispell the confusion.

Regarding exceptions, bluebird provides good debugging features for dev mode.

Practical debugging solutions such as unhandled rejection reporting, typed catches, catching only what you expect and very long, relevant stack traces without losing perf

Developers can switch to something lighter but slower for production, if needed. There are few use cases for speedy promises on the client side.

@Raynos
Copy link

Raynos commented Mar 19, 2014

Developers can switch to something lighter but slower for production

Do not recommend that. The place where you need the debugging features is production, not development. Debugging in development is trivial because you can reproduce things, restart your programs, make smaller test cases in your unit tests and add break points.

Debugging in production is really hard because you can't do anything, all you can do is rely on the things you said up pro actively. This is where bluebird comes in.

@lhorie
Copy link
Member

lhorie commented Mar 21, 2014

Just a quick update: I just took a quick look at promiscuous, but it uses setImmediate, which is only supported in Node and IE10. @domenic wrote a browser polyfill, but it clocks at >8kb, so overall not as small as I was hoping :(

The docs for setImmediate on the mozilla site say I could conceivably use setTimeout instead, but that comes w/ a 4ms minimum delay penalty per finalize call, which is unacceptable.

Gonna need to spend some more time w/ it to see if there's any way to refactor promiscuous, but as of now, an Angular-like "jQuery-lite" approach looks like the most feasible option at the moment.

@lhorie
Copy link
Member

lhorie commented Mar 21, 2014

This page clarifies a somewhat obscure point in the spec, which relates to the need for a setImmediate or similar mechanism. The rationale has to do with cleaning the call stack.

This means there's no way to refactor promiscuous that doesn't involve reimplementing the setImmediate polyfill

@Raynos
Copy link

Raynos commented Mar 22, 2014

@lhorie Also read http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony

A callback or promise must always be asynchronous. You will need setImmediate or some polyfill.

You can use https://github.com/medikoo/next-tick which might be smaller then the setImmediate polyfill.

@lhorie
Copy link
Member

lhorie commented Mar 22, 2014

Ok I think this is starting to become a bit of a rabbit hole. There are really three different issues here:

1 - throwing errors is not working as it should

2 - the current implementation does not guarantee that resolution is asynchronous

3 - it does not interop w/ 3rd party promises correctly (in the sense that you can't wrap others w/ Mithrils')

The first issue is the one originally being reported, and it's pretty serious because it makes the API semi-useless in a pretty obvious way. This is fixed in origin/next and scheduled to be released in v0.1.1

The other 2 issues aren't as pressing - given Mithril's scope, I doubt these issues are preventing people from writing apps and I don't want to hold up the release of the other bug fixes (some of which are quite visible and relatively important), so I'm going to fix only the reported issue for now and revisit the a+ compliance issue after v0.1.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

No branches or pull requests

8 participants