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

[DEPRECATION] Deprecate globals resolver #18436

Merged
merged 7 commits into from
Nov 19, 2019
Merged

[DEPRECATION] Deprecate globals resolver #18436

merged 7 commits into from
Nov 19, 2019

Conversation

Gaurav0
Copy link
Contributor

@Gaurav0 Gaurav0 commented Sep 27, 2019 •
edited
Loading

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)

@Gaurav0 Gaurav0 changed the title Deprecate globals resolver [DEPRECATION] Deprecate globals resolver Sep 27, 2019
@runspired
Copy link
Contributor

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

@Gaurav0 Gaurav0 force-pushed the deprecate_globals_resolver branch from 2c4938a to e545ec8 Compare October 27, 2019 10:01
rwjblue
rwjblue reviewed Nov 18, 2019
View reviewed changes
packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js Outdated Show resolved Hide resolved
packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js Outdated Show resolved Hide resolved
packages/@ember/application/globals-resolver.js Outdated Show resolved Hide resolved
packages/@ember/application/lib/application.js Outdated Show resolved Hide resolved
packages/@ember/application/tests/application_instance_test.js Outdated Show resolved Hide resolved
packages/@ember/application/tests/application_instance_test.js Outdated Show resolved Hide resolved
packages/@ember/application/tests/dependency_injection_test.js Outdated Show resolved Hide resolved
packages/ember/index.js Outdated Show resolved Hide resolved
packages/internal-test-helpers/lib/engine.js Outdated Show resolved Hide resolved
@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Nov 18, 2019

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

@Gaurav0 Gaurav0 force-pushed the deprecate_globals_resolver branch from e545ec8 to 74194e9 Compare November 18, 2019 06:34
@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Nov 18, 2019

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

@Gaurav0 Gaurav0 requested a review from rwjblue November 18, 2019 06:37
rwjblue
rwjblue reviewed Nov 18, 2019
View reviewed changes
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!

packages/@ember/application/tests/dependency_injection/default_resolver_test.js Outdated
@@ -198,6 +198,7 @@ moduleFor(
}

[`@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
Contributor Author

@Gaurav0 Gaurav0 Nov 18, 2019 •
edited
Loading

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.

packages/@ember/application/tests/dependency_injection_test.js Outdated
@@ -13,7 +13,12 @@ moduleFor(
constructor() {
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
Contributor 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.

packages/@ember/application/tests/dependency_injection/normalization_test.js Show resolved Hide resolved
packages/ember/index.js Show resolved Hide resolved
@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:

@Gaurav0 Gaurav0 requested a review from rwjblue November 18, 2019 18:38
rwjblue
rwjblue reviewed Nov 18, 2019
View reviewed changes
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.

packages/@ember/deprecated-features/index.ts Outdated Show resolved Hide resolved
@Gaurav0 @rwjblue
Correct version number.
e7b2c13 
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
Contributor Author

Gaurav0 commented Nov 19, 2019

Thank you @rwjblue !

@Gaurav0 Gaurav0 deleted the deprecate_globals_resolver branch November 19, 2019 22:32
@Gaurav0 Gaurav0 mentioned this pull request Nov 22, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 25, 2020
Merged
@mehulkar mehulkar mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gaurav0 @runspired @rwjblue