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

Drop autorun assertion #16797

Merged
merged 1 commit into from
Jul 13, 2018
Merged

Drop autorun assertion #16797

merged 1 commit into from
Jul 13, 2018

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Jul 6, 2018

This removes the autorun assertion in test mode. The original conditions that caused it to be added are no longer true, as we are on a version of backburner that uses microtask scheduling.

This removes the autorun assertion in test mode. The original conditions that caused it to be added are no longer true, as we are on a version of backburner that uses microtask scheduling.
@sukima
Copy link
Contributor

sukima commented Jul 6, 2018

This is the best!! I've been waiting for this for almost a year!! 🎉 @ef4 you rock! You are an inspiration.

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2018

I believe that there are other factors that we need to account for before we teach that using native async/await is ready for production app code usage (outside of the preexisting async route hooks). This change (removing autorun assertion) may be fine now that backburner itself is using microtask queue progression, but we should do some testing to be sure...

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2018

/cc @stefanpenner / @krisselden / @runspired since I know you all have strong thoughts on this topic...

@krisselden
Copy link
Contributor

@rwjblue I think there is work to be done to improve native promises settling before render but I think it is ok to do this now.

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2018

OK, seems good to me. I do think that we still need to be careful RE: suggesting folks use native async await in app code, but removing the assertion as the logical next step seems fine to me.

@rwjblue rwjblue merged commit 22c2826 into master Jul 13, 2018
@rwjblue rwjblue deleted the allow-autorun branch July 13, 2018 18:49
@runspired
Copy link
Contributor

@krisselden @rwjblue I think that what we want is either to change backburner onEnd or glimmer's schedule from onEnd to flush via the eventQueur/rAF (whichever comes first) and to not schedule if already scheduled. This would be the least breaking change that would also ensure native promises and repeated run-loops don't troll us for render perf.

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.

5 participants