[CLEANUP] Change resolver function deprecations in to assert #15911
Conversation
|
Can you create a corresponding PR (or issue that links back here) at https://github.com/emberjs/ember-2-legacy? |
|
@thoov - Do you think we need to ENV flag guard this one? How would we implement the legacy addon implementation otherwise? |
| @@ -24,10 +24,6 @@ export default class Registry { | |||
| this.fallback = options.fallback || null; | |||
| this.resolver = options.resolver || null; | |||
|
|
|||
| if (typeof this.resolver === 'function') { | |||
There was a problem hiding this comment.
I think we are going to have to put an ENV guard around this. Here is an example of how I just did a similar thing.
There was a problem hiding this comment.
Also here is an example of adding the config to ember-2-legacy
There was a problem hiding this comment.
@thoov @rwjblue I have updated the PR with the ENV guard and created a PR in ember-2-legacy to update the config with the ENV.
Please have a look.
|
@rwjblue Yes I think the simplest approach is to wrap it in an ENV. The only other ways would been to extend the registry past the point of it being worth the work. |
gowthamrm
force-pushed
the
remove-resolver-function-in-registry
branch
from
f37b492 to
6ca9e60
Compare
gowthamrm
changed the title
gowthamrm
force-pushed
the
remove-resolver-function-in-registry
branch
3 times, most recently
from
872d4b1 to
db3f7c8
Compare
| expect(2); | ||
|
|
||
| ENV._ENABLE_RESOLVER_FUNCTION_SUPPORT = true; |
There was a problem hiding this comment.
We will want to better reset this value. You can look at this example for how I was reseting the ENV properties in tests (https://github.com/emberjs/ember.js/pull/15903/files#diff-7c1b00291152c89da72c8940b7305c1a)
|
@gowthamrm One final thing then it looks good for me. Thanks! |
gowthamrm
force-pushed
the
remove-resolver-function-in-registry
branch
from
db3f7c8 to
fb9a715
Compare
|
@thoov Thanks for your comments, I have updated the test now. 👍 |
| @@ -489,6 +503,20 @@ QUnit.test('A registry can be created with a deprecated `resolver` function inst | |||
| equal(registry.resolve('foo:bar'), 'foo:bar-resolved', '`resolve` still calls the deprecated function'); | |||
| }); | |||
|
|
|||
| QUnit.test('A registry created with `resolver` function instead of an object throws assertion', function() { | |||
There was a problem hiding this comment.
do you mind updating to use the newer style with an assert argument here, and then assert.expect(1) below?
|
|
||
| let registry; | ||
|
|
||
| throws(() => { |
There was a problem hiding this comment.
This should be swapped to use expectAssertion (so that it works properly in production builds since Ember.assert does not throw there).
gowthamrm
force-pushed
the
remove-resolver-function-in-registry
branch
from
fb9a715 to
32fa778
Compare
|
@rwjblue Thank you, I have updated the test with the suggested changes, Please have a look. |
|
Thank you! |
No description provided.