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

[Deprecation]: implicit injections #19358

Merged
merged 5 commits into from
Feb 6, 2021
Merged

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Jan 26, 2021

Implements the implicit injections RFC

emberjs/rfcs#680
maybe ref #17791

@snewcomer snewcomer force-pushed the rfc-680 branch 2 times, most recently from 06af83b to 206379c Compare January 26, 2021 16:17
Copy link
Contributor Author

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Some comments for @pzuraq and @rwjblue

_internalName!: string;
_names: unknown;
_router: EmberRouter | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we force this to be just _router: EmberRouter;?

packages/@ember/-internals/utils/lib/inspect.ts Outdated Show resolved Hide resolved
@snewcomer snewcomer force-pushed the rfc-680 branch 4 times, most recently from 4211123 to 4f4b9f0 Compare February 2, 2021 03:09
packages/@ember/-internals/routing/lib/system/route.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/routing/lib/system/router.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/routing/lib/system/router.ts Outdated Show resolved Hide resolved
let factory = getFactoryFor(obj);
if (factory) {
for (let injection in factory.injections) {
if (factory.injections[injection] === properties[injection]) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this checking? What is properties here?

Copy link
Contributor

Choose a reason for hiding this comment

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

properties is Object.assign({}, injections, createProps) basically. So it's possible that the implicit injection has been overridden by an explicit value (an argument in a classic component for instance)

@@ -197,7 +211,9 @@ moduleFor(
assert.equal(result.foo.bar, 'foo');
}

['@test does raise deprecation if descriptor is a getter and not equal to the implicit deprecation'](assert) {
['@test does not raise deprecation if descriptor is a getter and not equal to the implicit deprecation'](
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this change intended? Based on the code changes, it seems so?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, after thinking about it some more my conclusion was that we can't really assert about getters, specifically because when a user defines a getter, they also must define a setter. Otherwise, the implicit injection will do nothing (or error, if JS strict mode is enabled). With that in mind, its possible that the user could define a setter that swallows the implicit injection, handling the deprecated behavior essentially.

Happy to discuss further if that doesn't make sense

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