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

nearly Promises/A+ compliant via Promiz.mithril.js #169

Merged
merged 2 commits into from Aug 16, 2014
Merged

nearly Promises/A+ compliant via Promiz.mithril.js #169

merged 2 commits into from Aug 16, 2014

Conversation

Zolmeister
Copy link
Contributor

Added a (nearly) Promises/A+ compliant implementation based off of Promiz.micro.js (the smallest Promises/A+ implementation, at 228bytes min + gzip)

I apologize for the whitespace edits, that was my editor.
Also note that I had to change two tests

This mostly addresses #1

@lhorie
Copy link
Member

lhorie commented Jul 22, 2014

Just curious, why is it only "nearly" compliant?

@Zolmeister
Copy link
Contributor Author

Unfortunately it is impossible to make it compliant because this implementation does not execute asynchronously:
2.2.4

onFulfilled or onRejected must not be called until the execution context stack contains only platform code.

For my purposes I don't mind it being this way (which is why I left it as is), though others may have stronger feelings about it.

@pygy
Copy link
Member

pygy commented Jul 22, 2014

See promises-aplus/promises-spec#4 and the issues referenced there for a thourough discussion. It is IIRC a matter of predictability and security.

If the resolution is allowed to be synchronous, it is possible for an attacker to make the promise resolve synchronously or asynchronously, potentially bypassing some initialization code in the first case.

I see that your micro implementation uses setTimer(fire) rathre than fire(), why don't you use the same strategy here?

@Zolmeister
Copy link
Contributor Author

@pygy, because the tests were written synchronously (backwards compatibility), and because of the 4-10ms delay caused by setTimeout, as commented here: #1 (comment)

@lhorie
Copy link
Member

lhorie commented Jul 22, 2014

@pygy Am I reading that wrong, or does the thread basically end with the conclusion that async is bad?

@pygy
Copy link
Member

pygy commented Jul 22, 2014

Some people are complaining, but the authors of the spec are very clear that async is the way to go.

The crux of the argument is apparently in the second comment of this thread: promises-aplus/promises-spec#139. You must read the first one for context. To be honest, I don't understand what erights means by invariants, but it convinced Domenico, so I suppose it makes sense :-).

@Raynos
Copy link

Raynos commented Jul 22, 2014

@pygy this article ( http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony ) clearly explains how an interface that is "somethings sync vs sometimes async" makes debugging hard.

It allows the introduction of "synchronous race conditions"

@pygy
Copy link
Member

pygy commented Jul 22, 2014

@Raynos: I understand the general point about race conditions, what I don't get is what erights means by "invariant".

@Raynos
Copy link

Raynos commented Jul 22, 2014

@pygy by invarient he means this.

function MyProto() {
  this.state = "cat"
  this.somePromise = new Promise();
}

MyProto.prototype.foo = function () {
  this.state = "dog";
  this.somePromise.resolve();
  assert.equal(this.state, "dog"); // synchronous resolution can mutate this.state
}

The invariant that an object's state is not mutated inside a method is broken if a promise sometimes resolves synchronously and sometimes asynchronously.

@pygy the design by contract article on wikipedia ( http://en.wikipedia.org/wiki/Design_by_contract ) has good reference on what it means to have "invariants upheld"

@pygy
Copy link
Member

pygy commented Jul 22, 2014

@Raynos Thanks for the clarifications :-)

Conflicts:
	mithril.js
@Zolmeister
Copy link
Contributor Author

@lhorie - Can we get this merged?

This was referenced Aug 13, 2014
@Zolmeister Zolmeister merged commit 7639444 into MithrilJS:next Aug 16, 2014
@pospi
Copy link

pospi commented Sep 22, 2014

Just to note- this fix also resolves an issue whereby promises swallow any errors in a promise success callback. I experienced this specifically with undefined variable errors in m.request callbacks but the issue does seem to be with an overzealous try/catch block in the current deferred implementation.

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

5 participants