-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Callback context is inconsistent #133
Comments
Generally, I agree with you that the context should be the same for all of the callbacks. Unfortunately, this is a breaking change and I am not sure how exactly we would go about deprecating the current behavior. @dgeb - Thoughts? |
I also agree. Having consistent context on all the methods is important for preserving the QUnit mental model. |
@rwjblue true, definitely breaking. Could we do something like proxy attributes from the test module onto the context? Would probably get ugly though. |
Yes, @mmun and I were chatting this through in slack and I came up with the following snippet, that should work in a backwards compatible way: buildDeprecatedContext(module, context) {
let keysForDeprecation = Object.keys(module);
if (keysForDeprecation.length === 0) {
return; // do not call Object.create if we don't need to
}
let wrappedContext = Object.create(context);
for (let index = 0, length = keysForDeprecation.length; index++; index < length) {
let key = keys[index];
Object.defineProperty(wrappedContext, key, {
get: function() {
Ember.deprecate('ffffuuuuuu, fix it!');
return module[key];
}
});
});
}, And call that from We would need tests for:
@trentmwillis - Think you would like to tackle this? |
I'd definitely be willing to work on this. One remaining question, how should we handle the situation where a user has defined something on the context that conflicts with the module? I'm thinking let the module "win" (as happens in your snippet), but wasn't sure if there is a better approach for that edge case. |
Yes, module should win with a more specific deprecation |
Currently when callbacks for a test module are contextualized, they get bound to the test module rather than the test context. This causes inconsistent behavior, since
setup
/teardown
get invoked with the test context.Practically, this means that if you do something such as:
changeTitle
won't work properly, instead you have to do:This is inconsistent and not intuitive. Can we change it such that all callbacks are invoked with the test context? This might limit access to some test module properties, but I feel most of those shouldn't be getting accessed anyway.
The text was updated successfully, but these errors were encountered: