kimroen / ember-cli-document-title

Adding document title behaviour to your ember app
MIT License
215 stars 61 forks source link

Add support for promise titleTokens #55

Closed poohitan closed 7 years ago

poohitan commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

poohitan commented 7 years ago

Added a test. Please check.

Duder-onomy commented 7 years ago

@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 commented 7 years ago

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

Duder-onomy commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

offirgolan commented 7 years ago

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

kimroen commented 7 years ago

@offirgolan Yes, but I need to land this PR first so it doesn't stop working in FastBoot: https://github.com/kimroen/ember-cli-document-title/pull/60

That PR is blocked by getting the tests set up properly: https://github.com/kimroen/ember-cli-document-title/pull/62

If you have ideas for workarounds to that problem I'm all ears!

offirgolan commented 7 years ago

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 commented 7 years ago

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!