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 A+ Compliance #3699

Closed
wants to merge 5 commits into from
Closed

Promises A+ Compliance #3699

wants to merge 5 commits into from

Conversation

@IgorMinar
Copy link
Member

IgorMinar commented Aug 22, 2013

No description provided.

@jamestalmage
Copy link
Contributor

jamestalmage commented Aug 22, 2013

Awesome!
See this branch. The file run-aplus-tests.html is a standard mocha browser runner (not hooked into the karma build). It verifies that $q promises are now a-plus compliant even when they are entirely outside the angular $digest loop.

My branch still makes use of the modified test-suite I created using component. The only problem I have with the way we are currently running the test suite (in Node), is that my promise-adapter isn't returning actual $q promises, but ones that uses process.nextTick instead of the $evalAsync for ensuring async execution.

@ghost ghost assigned jeffbcross and btford Aug 22, 2013
@IgorMinar
Copy link
Member Author

IgorMinar commented Aug 22, 2013

@jamestalmage I think that the nextTick thing is fine given that the test suite runs in node

@jeffbcross
Copy link
Contributor

jeffbcross commented Aug 22, 2013

Only issues I've noticed with this PR:

  • The spacing for new task configs in Gruntfile is four-spaced tabs instead of two-spaced
  • Two random docs tests are failing.

I'm not knowledgeable in the ways of A+ compliance, so I'd have to do some deeper digging to provide more valuable feedback.

@jamestalmage
Copy link
Contributor

jamestalmage commented Aug 22, 2013

The compliance test suite has an open issue regarding running in the browser: promises-aplus/promises-tests#15.

Once that is resolved, we can revisit running these tests in the browser.

@@ -680,6 +680,16 @@ function $RootScopeProvider(){
*
*/
$evalAsync: function(expr) {
// if we are outside of an $digest loop and this is the first time we are scheduling async task also schedule
// async auto-flush
if (!this.$$phase && !this.$$asyncQueue.length) {

This comment has been minimized.

Copy link
@mernen

mernen Aug 23, 2013

Contributor

this.$$phase is troublesome when you happen to have an isolated scope. Could this be replaced by $rootScope.$$phase?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Aug 23, 2013

Author Member

good point

@IgorMinar
Copy link
Member Author

IgorMinar commented Aug 23, 2013

there are some auxiliary test:docs tests failing because they don't expect anything else to populate mock $browser.defer queue. These tests are too strict and need to be fixed.

@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

@IgorMinar I'll take a look.

IgorMinar added 5 commits Aug 25, 2013
When calling $timeout.flush with or without a delay an exception should
be thrown if there is nothing to be flushed.

This prevents tests from flushing stuff unnecessarily.

BREAKING CHANGE: calling $timeout.flush(delay) when there is no task to be flushed
within the delay throws an exception now.

Please adjust the delay or remove the flush call from your tests as the exception
is a signed of a programming error.
When $timeout#flush is called with a delay and no task can be flushed within that
delay, the current time should not be updated as that gets the mock into an inconsistent
state.

BREAKING CHANGE: if a tests was written around the buggy behavior the delays might be off now

This would typically not be a problem, but because of the previous breaking change in
$timeout.flush, the combination of two might be confusing and that's why we are documenting
it.

Old behavior:

```
doSomething(); //schedules task to execute in 500ms from now
doOtherStuff(); //schedules task to execute in 600ms from now

try {
  $timeout.flush(300); // throws "no task to be flushed" exception
} catch(e) {};
$time.flush(200); //flushes only doSomething() task
```

New behavior:

```
doSomething(); //schedules task to execute in 500ms from now
doOtherStuff(); //schedules task to execute in 600ms from now

try {
  $timeout.flush(300); // throws "no task to be flushed" exception
} catch(e) {};
$time.flush(200); // throws "no task to be flushed" exception again
                  // because previous exception didn't move the time forward
```

Fixed test:

```
doSomething(); //schedules task to execute in 500ms from now
doOtherStuff(); //schedules task to execute in 600ms from now

try {
  $timeout.flush(300); // throws "no task to be flushed" exception
} catch(e) {};
$time.flush(500); // flushes only doSomething() task
```
This change causes a new $digest to be scheduled in the next tick if
a task was was sent to the $evalAsync queue from outside of a $digest
or an $apply.

While this mode of operation is not common for most of the user code,
this change means that $q promises that utilze $evalAsync queue to
guarantee asynchronicity of promise apis will now also resolve outside
of a $digest, which turned out to be a big pain point for some developers.

The implementation ensures that we don't do more work than needed and
that we coalese as much work as possible into a single $digest.

The use of $browser instead of setTimeout ensures that we can mock out
and control the scheduling of "auto-flush", which should in theory
allow all of the existing code and tests to work without negative
side-effects.

Closes #3539
Closes #2438
@IgorMinar
Copy link
Member Author

IgorMinar commented Sep 4, 2013

this has landed already before rc.2 was cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.