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

Add support for promise titleTokens #55

Merged
merged 2 commits into from
Sep 4, 2017
Merged

Conversation

poohitan
Copy link
Contributor

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.

@kimroen
Copy link
Owner

kimroen commented Mar 26, 2017

Thank you very much for looking at this - this is promising (no pun intended).

Would you mind adding a test verifying that having the titleToken function return a promise works?

Thanks!

@poohitan
Copy link
Contributor Author

Yes, I will add a test a bit later, sorry.

@poohitan
Copy link
Contributor Author

Added a test. Please check.

@Duder-onomy
Copy link

@kimroen Do you think this will be merged and a new release created any time soon?
We are pulling from @poohitan 's fork right now and would like to point back at this repo eventually.

@kimroen
Copy link
Owner

kimroen commented Apr 28, 2017

@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:

  • Does the route loading wait for the promise returned from the titleToken before rendering the page?
  • Do async title tokens work with FastBoot?

@Duder-onomy
Copy link

@kimroen

I cannot speak to FastBoot as I dont have that setup for any apps yet,
BUT! Adding promise support for titleToken does not block the page render,
I pulled down @poohitan's fork and took this gif, notice the page renders, then the title changes 3 seconds later:
ember-cli-document-title promise title token does not block render

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.

@poohitan
Copy link
Contributor Author

@kimroen I can't say anything about FastBoot, but I can confirm what @Duder-onomy said. Page doesn't wait for titleToken to resolve before rendering.

@Duder-onomy
Copy link

Just wanted to follow up here.
@kimroen For this to work nice with fastboot ( and I assume, get merged) , would we need to block render while the promise is unresolved?

@Duder-onomy
Copy link

Duder-onomy commented Aug 30, 2017

@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.
Anyhow, I keep getting tickets in my sprint to stop using @poohitan's fork and switch back to your repo, but we gotta get some promise support in here first.

@kimroen
Copy link
Owner

kimroen commented Sep 3, 2017

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.

@poohitan
Copy link
Contributor Author

poohitan commented Sep 4, 2017

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 :)

@kimroen
Copy link
Owner

kimroen commented Sep 4, 2017

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 ❤️

@kimroen kimroen merged commit b50c204 into kimroen:master Sep 4, 2017
kimroen added a commit that referenced this pull request Sep 4, 2017
kimroen added a commit that referenced this pull request Sep 10, 2017
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 added a commit that referenced this pull request Sep 11, 2017
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.
@offirgolan
Copy link

@kimroen any chance you can cut a release with this feature? 🙏

@kimroen
Copy link
Owner

kimroen commented Sep 18, 2017

@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!

@offirgolan
Copy link

Ah no worries at all. I'll just point to that commit for now.

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!

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.

@kimroen
Copy link
Owner

kimroen commented Sep 18, 2017

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!

kimroen added a commit that referenced this pull request Apr 2, 2018
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.
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.

Title token using an async relationship
4 participants