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

Remove asap package #969

Merged
merged 6 commits into from
Dec 12, 2019
Merged

Remove asap package #969

merged 6 commits into from
Dec 12, 2019

Conversation

jcranendonk
Copy link
Contributor

@jcranendonk jcranendonk commented Nov 12, 2019

The asap package (and its fork, falcor-asap) have occasionally caused issues with our build pipeline. We're not using the scheduler that relies on this package, so I'm removing it.

I kept the ASAPScheduler in the repository, in case anyone imports that file, but it simply forwards to new TimeoutScheduler(1) now. That scheduler may have slightly different behavior, but it should be similar enough for most use cases.

If you still need the exact asap behavior, there is a workaround: you can provide a custom scheduler to Falcor.Model.

Jeroen Cranendonk added 3 commits November 11, 2019 17:02
ASAPScheduler was replaced with TimeoutScheduler(1). It is kept in the
repository for backwards compatibility.
@coveralls
Copy link

coveralls commented Nov 12, 2019

Coverage Status

Coverage increased (+0.001%) to 92.598% when pulling fac46c1 on jcranendonk:remove-asap into 66984a8 on Netflix:master.

Copy link

@tquetano-netflix tquetano-netflix left a comment

Choose a reason for hiding this comment

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

Other than the question I left, LGTM.

};
// Retained for backwards compatibility
function ASAPScheduler() {
return TimeoutScheduler.bind(this, 1)();

Choose a reason for hiding this comment

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

Can't this just be TimeoutScheduler.call(this, 1);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...yes, it can 🤦‍♂

@jcranendonk jcranendonk merged commit 321bca8 into Netflix:master Dec 12, 2019
@jcranendonk jcranendonk deleted the remove-asap branch December 17, 2019 19:11
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

3 participants