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

Allow usage with ember-cli-fastboot. #13

Merged
merged 3 commits into from
Apr 22, 2015

Conversation

rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Apr 22, 2015

  • renderer:-dom is provided in versions of Ember > 1.10.
  • Actually test the real document.title in ember test (to help ensure this is really working).

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

Interesting, I wonder if we could test that it works with fastboot.

👍 on the changes!

@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

I wonder if we could test that it works with fastboot.

A few more changes are required in Fastboot itself to allow this, but I am working on those next. The first step was to remove the usage of window.document in that scenario (because window or document are undefined in Node-land).

@kimroen - Working on another PR to confirm that it works on all Ember versions from 1.10 and up.

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

@rwjblue Ah, using ember-try? Thanks.

@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

@kimroen - confirm

rwjblue added 2 commits April 22, 2015 10:21
* `renderer:-dom` is provided in versions of Ember > 1.10.
* Actually test the real document.title in `ember test` (to help ensure this is really working).
@rwjblue rwjblue force-pushed the enable-fast-boot-title branch from 77c36d2 to 762302a Compare April 22, 2015 14:40
@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

OK, this should be ready to go.

  • Added ember-try testing 1.10, 1.11, release, beta, and canary.
  • Added unit test for Router.prototype.setTitle.
  • Ensured that the document.title is cleaned up after each test.

Travis will now run one build per Ember version, so that a failure in a specific version can be spotted easily. This is in line with the current ember-cli blueprint for new addons (unreleased).

See the travis build here: https://travis-ci.org/kimroen/ember-cli-document-title/builds/59563596

},

afterEach: function() {
Ember.run(application, 'destroy');
document.title = document.title;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

document.title = originalTitle;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heheh, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Good catch.

@rwjblue rwjblue force-pushed the enable-fast-boot-title branch from 762302a to 80ed50c Compare April 22, 2015 15:01
@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

Cool - this is pretty rad. Thanks again.

I'll merge and release this in a little bit.

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

Just as a note: I'm not sure I understand what _dom.document.title does - will it set the <title> tag that is in the served index.html?

kimroen added a commit that referenced this pull request Apr 22, 2015
Allow usage with ember-cli-fastboot.
@kimroen kimroen merged commit 205c4d5 into kimroen:master Apr 22, 2015
kimroen added a commit that referenced this pull request Apr 22, 2015
@rwjblue rwjblue deleted the enable-fast-boot-title branch April 22, 2015 16:41
@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

It doesn't do anything.... Yet

@danmcclain
Copy link

@kimroen Yes, in client side ember, it ends up overwriting the title, but on the server (fastboot) side, it doesn't do anything, like @rwjblue just said

@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

Well, that wasn't exactly correct.

In a browser (as of 1.11 or 1.12) '_dom.document' is just 'window.document'.

In node it does not blow up (which is an improvement), but it doesn't cause the fastboot server to add the title to < head> yet. I'm still working on that part.

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

Okay, so it doesn't really "add support for Fastboot" then, but it will likely Just Work™ once that's taken care of. Just trying to make sure I don't lie in the changelog :)

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

Thanks again - available as version 0.1.0.

@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 22, 2015

@kimroen - Correct it does not add support, it allows this addon to be used if you are using Fastboot. Without these tweaks, having this addon in an application that wanted to use fastboot would throw an error (because window and document are undefined).

@kimroen
Copy link
Owner

kimroen commented Apr 22, 2015

@rwjblue Understood!

On Wed, Apr 22, 2015 at 8:09 PM, Robert Jackson notifications@github.com
wrote:

@kimroen - Correct it does not add support, it allows this addon to be used if you are using Fastboot. Without these tweaks, having this addon in an application that wanted to use fastboot would throw an error (because window and document are undefined).

Reply to this email directly or view it on GitHub:
#13 (comment)

@rwjblue
Copy link
Contributor Author

rwjblue commented Apr 23, 2015

A little followup here: using ember-fastboot/ember-cli-fastboot#22 along with 0.1.0 of ember-cli-document-title works properly when rendered via ember-cli-fastboot.

@kimroen
Copy link
Owner

kimroen commented Apr 23, 2015

Thanks for the update - very exciting!

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.

3 participants