-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add support for promise titleTokens #55
Conversation
Thank you very much for looking at this - this is promising (no pun intended). Would you mind adding a test verifying that having the Thanks! |
Yes, I will add a test a bit later, sorry. |
Added a test. Please check. |
@Duder-onomy I'm not sure when I'll be able to get this in, but some things I'd love to have someone check and confirm:
|
I cannot speak to FastBoot as I dont have that setup for any apps yet, As added evidence, TheDyrt.com (decently big Ember app) uses @poohitan's fork right now in production with no issues if that is any consolation. |
@kimroen I can't say anything about FastBoot, but I can confirm what @Duder-onomy said. Page doesn't wait for |
Just wanted to follow up here. |
@kimroen I hate to hound you. But do you know what it would take to get this working with fastboot how you expect it to? I suspect we need to block the fastboot render? I am not using fastboot at work, so I am not sure as to the expectations. |
Thanks again for this, I'll merge it shortly (I'll need to update the documentation, and I don't have time to do that right now). We should handle the Fastboot side of it separately, there are other issues unrelated to this that will need resolving either way. |
Hey @kimroen, I've just added a new section to docs. Please check if it's ok or I need to change or supplement it. Appreciate anyone's help :) |
Thank you again very much! There are some spelling mistakes and other things in there that I'd like to fix, but I'll merge it in as it is and work on it a bit separately. It's a great starting point, and I really appreciate it ❤️ |
Following up from #55, this makes FastBoot wait for our async code to complete before rendering the page. The code checks that FastBoot exists and that we’re running on FastBoot before using [fastboot.deferRendering](https://ember-fastboot.com/docs/user-guide#deferring-response-rendering) to tell FastBoot about our Promise.
Following up from #55, this makes FastBoot wait for our async code to complete before rendering the page. The code checks that FastBoot exists and that we’re running on FastBoot before using [fastboot.deferRendering](https://ember-fastboot.com/docs/user-guide#deferring-response-rendering) to tell FastBoot about our Promise.
@kimroen any chance you can cut a release with this feature? 🙏 |
@offirgolan Yes, but I need to land this PR first so it doesn't stop working in FastBoot: #60 That PR is blocked by getting the tests set up properly: #62 If you have ideas for workarounds to that problem I'm all ears! |
Ah no worries at all. I'll just point to that commit for now.
I wish I can be of help here but I have yet to play around with fastboot. I suggest asking some question on the fastboot channel in slack though. I'm sure the mods on there can help you a lot more than I can. |
It's not a FastBoot-specific issue, the problem is about the test setup, so it's more of an ember-cli and node issue. Feel free to follow the links to various issues and check it out - you might have an idea of what you'd want to see. Thanks for the suggestion! |
Following up from #55, this makes FastBoot wait for our async code to complete before rendering the page. The code checks that FastBoot exists and that we’re running on FastBoot before using [fastboot.deferRendering](https://ember-fastboot.com/docs/user-guide#deferring-response-rendering) to tell FastBoot about our Promise.
In this pull request I want to resolve #42.
It will be possible to return a promise from
titleToken()
function. Document title will be unset until all promises from tokens chain will be resolved.