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.

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

[CLEANUP] Remove Globals Resolver #19652

Merged
merged 2 commits into from Jul 31, 2021
Merged

[CLEANUP] Remove Globals Resolver #19652

merged 2 commits into from Jul 31, 2021

Conversation

btecu
Copy link
Contributor

@btecu btecu commented Jul 18, 2021

No description provided.

@mixonic mixonic mentioned this pull request Jul 18, 2021
58 tasks
@btecu btecu force-pushed the resolver branch 2 times, most recently from 5a0f14d to f2649ac Compare July 18, 2021 18:07
@mixonic
Copy link
Sponsor Member

mixonic commented Jul 20, 2021

@btecu there was a failure in the Node tests here, and this PR will require a rebase as well.

@btecu
Copy link
Contributor Author

btecu commented Jul 20, 2021

@mixonic I might need some help with the Node tests failure if you have some time.

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 22, 2021

@btecu the setup-app boilerplate, and many of the tests in tests/node/ are using deprecate API. Normally this is not permitted in the Ember test suite, and a test will fail if a deprecation is present without expectation. However these tests run in their own realm. Without that enforcement, it seems like they have fallen far off the best-practice path.

My commit adds a resolver to the app, thus avoiding the default resolver (which after your removal is just a hard failure).

edit: I also rebased

@btecu
Copy link
Contributor Author

btecu commented Jul 22, 2021

@mixonic thanks for the explanation and your help!

@mixonic mixonic requested review from rwjblue, pzuraq, ef4 and dgeb July 23, 2021 00:35
@property resolver
@public
*/
resolver: null,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is weird. I see it was added in btecu@7b0a7ec#diff-a7583c2d819fdabaff78778f93e5112adc888bc3026338522862f08ddfadb4abR364 (or https://github.com/emberjs/ember.js/pull/12685/files#diff-a7583c2d819fdabaff78778f93e5112adc888bc3026338522862f08ddfadb4abR377 in the PR) by @dgeb but I don't see any usage of it. What am I missing? It was added with a @deprecated flag in jsdoc, but I don't see what is firing the deprecation.

Copy link
Member

Choose a reason for hiding this comment

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

but I don't see what is firing the deprecation.

I wonder if the code firing the deprecation was removed prior to 3.0 without removing the deprecated prop? Regardless, it doesn't seem that the resolver class prop is referenced, so it seems safe to remove this now.

Copy link
Sponsor Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Left an item I'd appreciate another set of eyes on. I think this is all good.

Copy link
Member

@dgeb dgeb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @btecu and @mixonic for seeing this through.

@property resolver
@public
*/
resolver: null,
Copy link
Member

Choose a reason for hiding this comment

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

but I don't see what is firing the deprecation.

I wonder if the code firing the deprecation was removed prior to 3.0 without removing the deprecated prop? Regardless, it doesn't seem that the resolver class prop is referenced, so it seems safe to remove this now.

@mixonic mixonic merged commit 318cfc5 into emberjs:master Jul 31, 2021
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.

None yet

3 participants