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.

Sign up for GitHub

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

Jump to bottom

Change callback context to deprecate test module properties #136

Merged
merged 1 commit into from
Dec 16, 2015
Merged

Change callback context to deprecate test module properties #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 38 additions & 4 deletions lib/ember-test-helpers/test-module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,21 +246,25 @@ export default Klass.extend({
this.cachedCalls = this.cachedCalls || {};

var keys = (Object.keys || Ember.keys)(callbacks);
var keysLength = keys.length;

for (var i = 0, l = keys.length; i < l; i++) {
this._contextualizeCallback(context, keys[i]);
if (keysLength) {
var deprecatedContext = this._buildDeprecatedContext(this, context);
for (var i = 0; i < keysLength; i++) {
this._contextualizeCallback(context, keys[i], deprecatedContext);
}
}
},

_contextualizeCallback: function(context, key) {
_contextualizeCallback: function(context, key, callbackContext) {
var _this = this;
var callbacks = this.callbacks;
var factory = context.factory;

context[key] = function(options) {
if (_this.cachedCalls[key]) { return _this.cache[key]; }

var result = callbacks[key].call(_this, options, factory());
var result = callbacks[key].call(callbackContext, options, factory());

_this.cache[key] = result;
_this.cachedCalls[key] = true;
Expand All @@ -269,6 +273,36 @@ export default Klass.extend({
};
},

/*
Builds a version of the passed in context that contains deprecation warnings
for accessing properties that exist on the module.
*/
_buildDeprecatedContext: function(module, context) {
var deprecatedContext = Object.create(context);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid the Object.create(context) call when there are no keysForDeprecation, can you tweak this to be behind a guard?

Something like this slight reordering:

var keysForDeprecation = Object.keys(module);

if (keysForDeprecation.length === 0) {
  return this;
}

var deprecatedContext = Object.create(context);
// .... other stuff

Copy link
Member

Choose a reason for hiding this comment

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

Wait, sorry, I misread. This is only being called when needed (because _contextualizeCallback is called only when needed), but we can still cache the deprecated context (instead of building a new one for every function that is in options).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will adjust this to cache the deprecatedContext


var keysForDeprecation = Object.keys(module);

for (var i = 0, l = keysForDeprecation.length; i < l; i++) {
this._proxyDeprecation(module, deprecatedContext, keysForDeprecation[i]);
}

return deprecatedContext;
},

/*
Defines a key on an object to act as a proxy for deprecating the original.
*/
_proxyDeprecation: function(obj, proxy, key) {
if (typeof proxy[key] === 'undefined') {
Object.defineProperty(proxy, key, {
get: function() {
Ember.deprecate('Accessing the test module property "' + key + '" from a callback is deprecated.', false, { id: 'ember-test-helpers.test-module.callback-context', until: '0.6.0' });
Copy link
Member

Choose a reason for hiding this comment

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

The until value here is tricky. I agree that 0.6.0 is the correct version number of this project, but none of this projects consumers actually see this version. For example, ember-mocha is already on 0.8.

It is likely that nothing can be done here, but I wanted to call it out and see if y'all had any ideas.

/cc @mixonic @dgeb

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Oof. Libraries outside of Ember core using deprecate is going to be painful for sure. There is no project property, which there probably should be if we really consider them a public API to be used when deprecating other libs.

Tricky b/c until is required.

Copy link
Member

Choose a reason for hiding this comment

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

I think the name spacing that we have done (and is done here) helps a bit. It at least says ember-test-helpers so you can know what to look for. Perhaps we should add a project option here, and add it to the API for the debug handlers feature so that various tools have a better chance at this...

Copy link
Member

Choose a reason for hiding this comment

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

Either way, I think this PR is doing the best we can ATM.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Agreed.

return obj[key];
}
});
}
},

_setupContainer: function(isolated) {
var resolver = getResolver();

Expand Down
36 changes: 34 additions & 2 deletions tests/test-module-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function setupRegistry() {
});
}

var callbackOrder, setupContext, teardownContext, beforeSetupContext, afterTeardownContext;
var callbackOrder, setupContext, teardownContext, beforeSetupContext, afterTeardownContext, originalDeprecate;

moduleFor('component:x-foo', 'TestModule callbacks', {
beforeSetup: function() {
Expand Down Expand Up @@ -94,13 +94,45 @@ moduleFor('component:x-foo', 'component:x-foo -- callback context', {
setupRegistry();
},

setup: function() {
originalDeprecate = Ember.deprecate;
},

teardown: function() {
Ember.deprecate = originalDeprecate;
},

getSubjectName: function() {
return this.subjectName;
},

getFoo: function() {
return this.foo;
}
});

test("callbacks are called in the context of the module", function() {
test("can access TestModule properties from a callback but raises a deprecation", function() {
var deprecations = [];
Ember.deprecate = function(message) {
deprecations.push(message);
};

equal(this.getSubjectName(), 'component:x-foo');
ok(Ember.A(deprecations).contains('Accessing the test module property "subjectName" from a callback is deprecated.'));
});

test("can access test context properties from a callback's 'this' and not raise a deprecation", function() {
var deprecations = [];
Ember.deprecate = function(message, test) {
if (!test) {
deprecations.push(message);
}
};

this.foo = 'bar';

equal(this.getFoo(), 'bar');
ok(!deprecations.length);
Copy link
Member

Choose a reason for hiding this comment

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

I am unsure why, but this assertion is failing on ember-1.13. I am rerunning in Travis in case it is a fluke.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that 1.13 is throwing a deprecation unrelated to the changes here. Will investigate this before updating again.

});

moduleFor('component:x-foo', 'component:x-foo -- created subjects are cleaned up', {
Expand Down