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

Fix visit() promise resolution #16059

Closed
krisselden opened this issue Jan 5, 2018 · 4 comments · Fixed by #16087
Closed

Fix visit() promise resolution #16059

krisselden opened this issue Jan 5, 2018 · 4 comments · Fixed by #16087
Assignees
Labels

Comments

@krisselden
Copy link
Contributor

krisselden commented Jan 5, 2018

The fix in this PR makes the issue worse, it is now less likely render is fully resolved when the visit promise is resolved.

#14591

This issue causes a lot of tests to force flush the run loop to ensure visit is done rendering.

To clarify, this issue isn't about error handling, it is about actually resolving the promise only when rendering is actually settled.

@tomdale
Copy link
Member

tomdale commented Jan 5, 2018

@krisselden Can you provide some more context about this bug?

The fix in this PR makes the issue worse, it is now less likely render is fully resolved when the visit promise is resolved.

What does "it" refer to here?

What are the specific behaviors that we need to change about the current implementation for us to consider promise resolution fixed and close this bug?

@krisselden
Copy link
Contributor Author

@krisselden
Copy link
Contributor Author

Also, I think it would be nice to have an API for a promise when rendering has settled.

@rwjblue
Copy link
Member

rwjblue commented Jan 7, 2018

As a mechanism to flush out tests that artificially forced visit to behave synchronously, I swapped Ember.ApplicationInstance.prototype.visit out to use run.next instead of run.schedule('afterRender', ....) (temporarily) and then fixed all the test failures. That work is progressing in #16086.

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

Successfully merging a pull request may close this issue.

3 participants