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

fastboot compatibility #67

Merged
merged 7 commits into from
Oct 12, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions addon/mixins/fastboot-compat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Mixin from 'ember-metal/mixin';
import getOwner from 'ember-getowner-polyfill';
import computed from 'ember-computed';

export default Mixin.create({
_fastboot: computed(function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Weird indenting here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correcting.

let owner = getOwner(this);
return owner.lookup('service:fastboot');
}),

_isFastBoot: computed.readOnly('_fastboot.isFastBoot')
});
7 changes: 6 additions & 1 deletion addon/services/scroll-activity.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import $ from 'jquery';
import Evented from 'ember-evented';
import Service from 'ember-service';
import run from 'ember-runloop';
import FastBootCompatMixin from '../mixins/fastboot-compat';

/*
* Polling uses rAF and/or a setTimeout at 16ms, however rAF will run in the
Expand All @@ -14,11 +15,13 @@ import run from 'ember-runloop';
*/
const MAX_POLL_PERIOD = 32;

export default Service.extend(Evented, {
export default Service.extend(Evented, FastBootCompatMixin, {

init() {
this._super(...arguments);

if (this.get('_isFastBoot')) { return; }

this._animationFrame = null;
this._subscribers = [];
this._lastCheckAt = new Date();
Expand Down Expand Up @@ -57,6 +60,7 @@ export default Service.extend(Evented, {
},

_pollScroll() {
if (this.get('_isFastBoot')) { return; }
if (window.requestAnimationFrame) {
this._animationFrame = requestAnimationFrame(() => this._checkScroll());
} else {
Expand Down Expand Up @@ -100,6 +104,7 @@ export default Service.extend(Evented, {
},

willDestroy() {
if (this.get('_isFastBoot')) { return; }
if (window.requestAnimationFrame) {
cancelAnimationFrame(this._animationFrame);
} else {
Expand Down
6 changes: 5 additions & 1 deletion addon/services/user-activity.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import injectService from 'ember-service/inject';
import { A } from 'ember-array/utils';
import { isEmpty } from 'ember-utils';
import { throttle } from 'ember-runloop';
import FastBootCompatMixin from '../mixins/fastboot-compat';


const { testing } = Ember;

export default Service.extend(Evented, {
export default Service.extend(Evented, FastBootCompatMixin, {
scrollActivity: injectService('ember-user-activity@scroll-activity'),

EVENT_THROTTLE: 100,
Expand Down Expand Up @@ -38,6 +40,7 @@ export default Service.extend(Evented, {
this.get('scrollActivity').on('scroll', this, this._handleScroll);
} else if (this._eventsListened.indexOf(eventName) === -1) {
this._eventsListened.push(eventName);
if (this.get('_isFastBoot')) { return; }
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably move this check higher...I'm not sure it makes sense for us to mark that an event is being listened to (via push) if we're not actually going to call addEventListener

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before push? Or on entering the function?

Copy link
Owner

Choose a reason for hiding this comment

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

Before push seems fine, since the scrollActivity service has its own code to handle fastboot compat

window.addEventListener(eventName, this._boundEventHandler, true);
}

Expand Down Expand Up @@ -83,6 +86,7 @@ export default Service.extend(Evented, {
if (eventName === 'scroll') {
this.get('scrollActivity').off('scroll', this, this._handleScroll);
} else {
if (this.get('_isFastBoot')) { return; }
window.removeEventListener(eventName, this._boundEventHandler, true);
}
},
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"ember-cli-uglify": "^1.2.0",
"ember-disable-prototype-extensions": "^1.1.0",
"ember-export-application-global": "^1.0.5",
"ember-getowner-polyfill": "1.0.1",
"ember-load-initializers": "^0.5.1",
"ember-resolver": "^2.0.3",
"ember-sinon": "0.5.1",
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/mixins/fastboot-compat-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Ember from 'ember';
import FastbootCompatMixin from 'ember-user-activity/mixins/fastboot-compat';
import { module, test } from 'qunit';

module('Unit | Mixin | fastboot compat');

// Replace this with your real tests.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a test here that just registers a mock fastboot service, and another one that doesn't (and confirms that isFastBoot is false as a result)>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually wanted to ask this question originally. But yes I will certainly do that. Thanks for the review!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fastboot service is only available when it is injected by the fastboot server, so this.get('this.get('_isFastBoot') will be undefined, ( the computed property _fastboot will return undefined) in the browser context. To verify the existence of the fastboot service a test would need to be written that gets run as a result of being contained within a fastboot server instance, like using https://github.com/kaliber5/ember-fastboot-addon-tests .

Copy link
Owner

Choose a reason for hiding this comment

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

You don't need the real fastboot service, you can simply inject a mock service called fastboot that has an _isFastBoot property. For the non-FastBoot test, you can literally just do nothing and check that the CP evaluates to false (undefined) as expected

test('it works', function(assert) {
let FastbootCompatObject = Ember.Object.extend(FastbootCompatMixin);
let subject = FastbootCompatObject.create();
assert.ok(subject);
});