Skip to content

Conversation

@Gaurav0
Copy link

@Gaurav0 Gaurav0 commented Sep 27, 2019

For emberjs/rfc-tracking#20

  • Deprecate instantiating @ember/application/globals-resolver (in init of packages/@ember/application/globals-resolver.ts)
  • Deprecate using Ember.Resolver / Ember.DefaultResolver (in packages/ember/index.js)

@runspired
Copy link
Contributor

YEAAAAAASSSSSSSSSSSS cc @scalvert I was just complaining to you that this still existed the other day :D

@Gaurav0
Copy link
Author

Gaurav0 commented Nov 18, 2019

Thank you for reviewing and providing feedback, @rwjblue . I'm working on this.

@Gaurav0
Copy link
Author

Gaurav0 commented Nov 18, 2019

@rwjblue Updated. I hope this alleviates most of your concerns.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

}

[`@test no assertion for routes that extend from Route`](assert) {
assert.test.assertions = []; // clear assertions that occurred in beforeEach
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why do we need to clear the assertions?

Copy link
Author

@Gaurav0 Gaurav0 Nov 18, 2019

Choose a reason for hiding this comment

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

In beforeEach we are creaing an application using the default resolver. This throws off the following assert.expect statements.

super();

application = run(EmberApplication, 'create');
// Must use default resolver because test resolver does not normalize
Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, so lets split the tests that rely on the globals resolver into their own moduleFor + class (e.g. moduleFor('Application Dependency Injection - Globals Resolver [DEPRECATED]', class extends TestCase {})).

Copy link
Author

Choose a reason for hiding this comment

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

There's only one test that relies on it, so I'm just going to move it into default_resolver_test.js.

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2019

I think we also need to add some svelting conditionals (so we can strip the deprecated code paths). The basic steps for this are:

  • add another entry to @ember/deprecated-features
  • import that flag in any file that implements the deprecated behavior (I think this is mostly the location that the globals resolver itself is defined)
  • wrap the implementation code in an if (GLOBALS_RESOLVER) {

Some example PRs that have landed that might be helpful:

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Looks great, only one minor nit-pick left.

Co-Authored-By: Robert Jackson <me@rwjblue.com>
@rwjblue rwjblue merged commit 7b9f0af into emberjs:master Nov 19, 2019
@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2019

Thank you @Gaurav0!

@Gaurav0
Copy link
Author

Gaurav0 commented Nov 19, 2019

Thank you @rwjblue !

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