Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix(spec): fakeAsyncTestSpec should handle requestAnimationFrame #805

Merged
merged 1 commit into from
Jun 11, 2017

Conversation

vikerman
Copy link
Contributor

@vikerman vikerman commented Jun 7, 2017

Fixes #804.

default:
task = delegate.scheduleTask(target, task);
throw new Error('Unknown macroTask scheduled in fake async test: ' + task.source);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you mention this fix in the commit description as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vikerman , @juliemr, I have one question that if we throw Error, unknown macroTask will not be able to be run in fakeAsyncZone, currently beside setTimeout/Interval and requestAnimationFrame, other zone patched macroTasks include setImmediate and nodejs fs/crypto methods, should we also make them available in the fakeAsyncTest?

thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that we should consider whether it's meaningful to make them available - but in the meantime, throwing an error is much more useful and clearer than silently passing through.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliemr, thank you for reply.

I'd say that we should consider whether it's meaningful to make them available - but in the meantime, throwing an error is much more useful and clearer than silently passing through.

Got it! I agree, if currently we don't need to make those API available, throw error is more making sense.

@mhevery
Copy link
Contributor

mhevery commented Jun 9, 2017

LGTM, @vikerman can you address @JiaLiPassion concern?

@mhevery mhevery merged commit 8260f1d into angular:master Jun 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants