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

support promise return in ospec #1928

Merged
merged 4 commits into from Aug 31, 2017

Conversation

Projects
None yet
5 participants
@StephanHoyer
Copy link
Member

StephanHoyer commented Aug 6, 2017

Support promise returns in ospec as alternative to done callback. Also supports async/await automatically

Description

Currently ospec does not support returning promises or using async/await without wrapper. This PR tries to add support for that.

Motivation and Context

async/await is the future for async stuff. We should support this.

How Has This Been Tested?

  • Wrote a test that verifies basic usage
  • checkt if timeouts work

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@StephanHoyer StephanHoyer requested a review from lhorie as a code owner Aug 6, 2017

@StephanHoyer StephanHoyer referenced this pull request Aug 6, 2017

Closed

suport promise return in ospec #1927

4 of 10 tasks complete

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch from 4b29f4e to 67b404a Aug 6, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 6, 2017

A glitch in the Matrix?

Edit: @StephanHoyer seriously, I don't understand why you reopened this (I'm a bit tired though)... aPromise.then(done)/await aPromise; done() doesn't cut it?

}
}
else {
fn()
nextTickish(next)

This comment has been minimized.

@pygy

pygy Aug 6, 2017

Member

IIRC there was a good reason to use nextTickish() here.

This comment has been minimized.

@StephanHoyer

StephanHoyer Aug 6, 2017

Member

readd

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch from 67b404a to ce06a34 Aug 6, 2017

@StephanHoyer StephanHoyer requested a review from tivac as a code owner Aug 6, 2017

var timeout = 0, delay = 200, s = new Date
var isDone = false

function done() {

This comment has been minimized.

@StephanHoyer

StephanHoyer Aug 6, 2017

Member

support done(err)

This would be breaking right?

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 7, 2017

Collaborator

Technically, yes, but it's basic functionality IMHO. Even Tape supports async errors (although indirectly in that case), and blue-tape handles promises gracefully and accordingly.

@@ -84,40 +84,56 @@ module.exports = new function init(name) {
if (cursor === fns.length) return

var fn = fns[cursor++]
var timeout = 0, delay = 200, s = new Date
var isDone = false

This comment has been minimized.

@pygy

pygy Aug 6, 2017

Member

Danger prevents the test suite from running, but I wonder how much time it takes to create these closures unconditionally?

if (fn.length > 0) { could become

var res; if (fn.length > 0 || (res = fn(), res != null && typeof res.then === 'function;)) {

And in that if block, assign done to a variable and, either call fn(done) or res.then(done)...

If it doesn't make sense don't worry, I'll have to read this with a fresh mind :-)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 7, 2017

@pygy This was opened fresh because the old one's branch has already been deleted.

Also, here's the background on why it's not just promise.then(done). Ignoring errors is a very bad idea in any context, but especially so for a test runner.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 7, 2017

This should shut danger up.

c29e269

Sorry about that :(

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch from ce06a34 to 1424142 Aug 7, 2017

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 7, 2017

@tivac just rebased... there seems to be an authentication issue now.

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 7, 2017

@pygy I reopened it, because @isiahmeadows suggested real async error handling would be a good thing to have. Couldn't open the former PR, github does not allow me to republish a deleted branch for some unknown reason. Thats why I opened another PR.

@MithrilJS MithrilJS deleted a comment from botvac Aug 7, 2017

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 7, 2017

Fixed danger, though its comment about a missing change-log.md entry is 100% valid.

Also you've got some lint errors?

/home/travis/build/MithrilJS/mithril.js/ospec/ospec.js

  129:20  error  Expected parentheses around arrow function argument  arrow-parens

/home/travis/build/MithrilJS/mithril.js/ospec/tests/test-ospec.js

  187:32  error  Parsing error: Unexpected token function

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch 2 times, most recently from 2012c6a to a963986 Aug 8, 2017

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 8, 2017

Re-ran the build, just in case it was transient. Still tossing the same errors about Promise chaining.

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 8, 2017

Yes

I'll try to fix this

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 8, 2017

as I said, it's a breaking change

@StephanHoyer StephanHoyer reopened this Aug 8, 2017

@StephanHoyer StephanHoyer changed the title suport promise return in ospec support promise return in ospec Aug 8, 2017

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch from 88e70a2 to cc0c882 Aug 8, 2017

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 10, 2017

Can anyone give me some feedback on this?

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 11, 2017

What's the breaking change? I'm not super-familiar with the ospec API apparently.

@tivac

tivac approved these changes Aug 11, 2017

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Aug 11, 2017

done callback accepts an err as first argument now. One of mithrils promise tests has to be adapted to the behaviour.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 11, 2017

I'm 👍 on that anyways.

@lhorie @isiahmeadows @pygy anyone else want to weigh in? I'll merge when I get back from vacation otherwise.

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Aug 11, 2017

LGTM 👍

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM mod nit.

@@ -131,7 +131,7 @@ o.spec("promise", function() {
callAsync(function() {resolve(promise)})
})

promise.then(null, done)
promise.then(null, () => done())

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 13, 2017

Collaborator

My one and only nit: use ES5-style functions.

This comment has been minimized.

@StephanHoyer

StephanHoyer Aug 15, 2017

Member

@isiahmeadows only here or everywhere?

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 15, 2017

Collaborator

For this, only in places where you have a diff. The rest should be flagged in a separate issue, if you'd like to file one for that. (The polyfill tests should be runnable in an ES5 environment for obvious reasons.)

This comment has been minimized.

@StephanHoyer

StephanHoyer Aug 18, 2017

Member

I mean, I have some other arrows in this PR. Should I convert them too? I assume yes.

This comment has been minimized.

@tivac

tivac Aug 18, 2017

Member

Yes, IMO tests are fine to use arrows but library has to use ES5 funcs for compat w/o transpilation.

This sounds like a good danger rule... 😄

This comment has been minimized.

@StephanHoyer

StephanHoyer Aug 31, 2017

Member

I converted one arrow fn in ospec to a fn-expression. Left the tests untouched.

Deferring to @tivac on his judgement of arrow functions in the promise polyfill tests

@StephanHoyer StephanHoyer force-pushed the StephanHoyer:ospec-promise-support branch 2 times, most recently from 5561b0b to 6fd8a52 Aug 31, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 31, 2017

@tivac Safe to merge?

@tivac

tivac approved these changes Aug 31, 2017

Copy link
Member

tivac left a comment

Arrow fns in tests are fine-ish, if it becomes a problem for tracking down an IE bug or something we can revisit.

@isiahmeadows isiahmeadows merged commit 57b6b51 into MithrilJS:next Aug 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@StephanHoyer StephanHoyer deleted the StephanHoyer:ospec-promise-support branch Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment