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 setting title in fastboot with glimmer2 #53

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

kratiahuja
Copy link

@kratiahuja kratiahuja commented Oct 10, 2016

#49 tried to fix glimmer 2 but it wasn't correct.

Calling renderer._env.getDOM() in fastboot environments gives back undefined. Ember now allows you to get the DOM object (whether SimpleDOM in fastboot environment or DOM in browser).

This PR fixes setting title propertly in fastboot environment for apps using 2.9.beta.x. The earlier check was incorrect.

PS: I am keeping the else if (domForAppWithGlimmer2) since not all apps may be on 2.9.beta.x that will contain the document service.

cc: @chadhietala @kimroen @workmanw

Tested this with fastboot app.

@workmanw
Copy link
Contributor

Thanks for fixing this. I haven't been using fastboot, so it didn't occur to me that this might be different.

@kimroen
Copy link
Owner

kimroen commented Oct 10, 2016

Thank you for this. I'm going to merge and release this now, but some changes are coming to this code in #38. Hopefully setting up tests (#52) first will make sure it doesn't break again.

@kimroen kimroen merged commit 37e0406 into kimroen:master Oct 10, 2016
kimroen added a commit that referenced this pull request Oct 10, 2016
@kimroen
Copy link
Owner

kimroen commented Oct 10, 2016

Released as [email protected].

@kratiahuja
Copy link
Author

Thank you @kimroen!

@chriskrycho
Copy link

chriskrycho commented Nov 21, 2016

This is actually a breaking change: I'm not sure when the service:-document private service got added, but it looks like it's not in 2.6 (which we're upgrading to currently), and as a result this change is breaking our app. 😬

We can of course target 0.3.2 in the meantime, but would you mind yanking 0.3.3 and republishing as 0.4.0? (I see you have other, semi-related changes coming in #38, but it won't fix this for us, or for anyone else who is using this add-on.)

Edit: notably, this is breaking our tests; we'll follow-up and confirm if it's breaking the app proper. It may be a week or so.

@kimroen
Copy link
Owner

kimroen commented Nov 21, 2016

This is actually a breaking change: I'm not sure when the service:-document private service got added, but it looks like it's not in 2.6 (which we're upgrading to currently), and as a result this change is breaking our app. 😬

Ouch, I'm really sorry about that. Is it breaking using FastBoot, or just running the app normally?

It could be that this happened because Glimmer didn't ship with 2.6 after all?

We can of course target 0.3.2 in the meantime, but would you mind yanking 0.3.3 and republishing as 0.4.0? (I see you have other, semi-related changes coming in #38, but it won't fix this for us, or for anyone else who is using this add-on.)

I don't mind at all, but I haven't yanked a release before so I'm not sure what the proper way is. Release 0.3.4 with something that works, and then release the change again as 0.4.0 and then mark 0.3.3 as a broken release?

Does anyone here know? I'll ask around.

@chriskrycho
Copy link

It’s breaking the app running normally, and 2.6 is relatively old at this point; we just hit it in the upgrade process as we’re trying to step up to 2.9 from 2.5.

I’m not sure what the process is, actually; I just know it’s possible. Thanks for getting back to me about it!

@kimroen
Copy link
Owner

kimroen commented Nov 21, 2016

Oh, 2.6! I totally read that as 2.9 - sorry again. I'll look into it.

@kratiahuja
Copy link
Author

I believe service:-document exists on the Application Instance since Ember 2.6.x. Here is a twiddle that showcases that.

@chriskrycho
Copy link

Interesting. I’ll follow up when we’re able to come back to it and let you know what the deal is!

@kimroen kimroen mentioned this pull request Nov 21, 2016
@kimroen
Copy link
Owner

kimroen commented Nov 21, 2016

I made an issue for this: #54

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.

4 participants