[CLEANUP beta] Deprecate owner.inject by snewcomer · Pull Request #19680 · emberjs/ember.js

snewcomer · 2021-07-25T17:27:37Z

See https://github.com/emberjs/rfcs/blob/sn/owner-inject-deprecation/text/0680-implicit-injection-deprecation.md#1-deprecate-implicit-injection-on-target-object for the reasons why we first added the initial deprecation in 3.26.0 and are now adding another deprecation but removing the functionality of app.inject.

deprecate ability to inject properties on Ember framework objects
Remove infrastructure supporting injection
remove deprecations ending in 4.0.0 from [Deprecation]: implicit injections #19358
remove blueprint inject function
remove all implicit injections in codebase

ref #19617

mixonic · 2021-08-17T02:57:43Z

@snewcomer can the cleanup/removal of the old API land in its own PR with [CLEANUP beta]? I worry this has entangled the removals with the addition of a deprecation, but they should not be coupled.

snewcomer · 2021-08-17T04:11:00Z

@mixonic So this timeline took me a bit to grep but is laid out in this RFC .

3.26 and up deprecated the use of implicit injections. See https://deprecations.emberjs.com/v3.x#toc_implicit-injections. So by now, everybody who is using implicit injections has been warned and hopefully made adjustments (including users of ember-data who relied on the implicit store deprecation).
4.x deprecates owner.inject. In addition, we will remove the supporting infrastructure to manage injections. inject is now effectively now a noop.
5.x will remove the injection function and supporting code.

Sry if the above was rehashing something you already know. But the deprecation + removal of the supporting infrastructure for inject go hand in hand.

@pzuraq noted we could assert as well. I think that was only if we didn't wrap up the work that is in this PR in time.

snewcomer · 2021-08-17T04:20:46Z

packages/@ember/-internals/glimmer/tests/integration/outlet-test.js

-      this.component = CoreOutlet.create();
+      let outletTemplateFactory = this.owner.lookup('template:-outlet');
+      let environment = this.owner.lookup('-environment:main');
+      this.component = CoreOutlet.create({ environment, template: outletTemplateFactory });


☝️ this is important to note. This was refactored from the implicit injection.

I believe this needs to be ported to @ember/test-helpers https://github.com/emberjs/ember-test-helpers/blob/9219247259ef6b4d8a339ca71a1374e2ffe69603/addon-test-support/%40ember/test-helpers/setup-rendering-context.ts#L266

working on that now.

emberjs/ember-test-helpers#1110

landed and released as @ember/test-helpers@2.4.2

pzuraq

So I believe that _lazyInjections can also be removed as part of this work. As far as I can tell, it's basically creating and setting up lazy versions of implicit injections, only in DEBUG, and I can't see any usage in the ecosystem at all: https://emberobserver.com/code-search?codeQuery=_lazyInjections

It's explicitly marked as a private API as well, so should be fine to remove. Overall this PR looks amazing and once those changes are made, I think we're good to merge!

packages/@ember/-internals/container/lib/registry.ts

packages/@ember/-internals/glimmer/lib/renderer.ts

snewcomer · 2021-08-17T15:51:37Z

tests/node/visit-test.js

-      assertResources('/b', ['/b', '/c']),
-      assertResources('/c', ['/c']),
-      assertResources('/d', ['/d', '/e']),
-      assertResources('/e', ['/e']),


I don't think the intention here was parallel. So I made it serial. This was also due to the fact that creating a singleton service per app instance that tracked "network" requests in each route was proving difficult when they happen in parallel. Lmk if anybody has any comments here.

snewcomer · 2021-08-17T16:33:55Z

tests/docs/expected.js

-    '_injections',
    '_internalReset',
    '_invoke',
    '_lazyInjections',


I checked out lazyInjections and it looks like it is used to validate service injections and not implicit.

ember.js/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js

Line 2898 in b818f54

['@test injecting an unknown service raises an exception']() {

mixonic · 2021-08-18T01:39:56Z

@snewcomer I don't understand the statement:

4.x deprecates owner.inject. In addition, we will remove the supporting infrastructure to manage injections. inject is now effectively now a noop.

Deprecated functionality is not removed, it is merely warned against in development builds.

Do you mean "if a user were to use an explicit injection as suggested by the deprecation message, then any owner.inject based type-based injection for the same property could be safely considered a no-op and removed" or some other phrasing?

edit: Ok, I see what you're going for.

packages/@ember/-internals/container/lib/registry.ts

mixonic · 2021-08-20T01:37:38Z

packages/@ember/-internals/container/lib/registry.ts

+      {
+        id: 'remove-owner-inject',
+        until: '5.0.0',
+        url: 'https://deprecations.emberjs.com/v3.x#toc_implicit-injections',


This should get a 4.x deprecation entry. The 3.x one is much more information than is probably needed, explains how to move away from APIs which the user already shouldn't be using, and is just confusing as it is in the 3.x series.

ember-learn/deprecation-app#932

If you can update the link here to point to the correct 4.x deprecation page we can land this PR independent of ember-learn/deprecation-app#932, IMO

mixonic

Awesome stuff here @snewcomer !

mixonic · 2021-08-22T18:44:25Z

Force pushed a single commit for a clean history, letting CI run.

stefanpenner · 2021-08-23T18:00:46Z

I believe this currently incompatible with @ember/test-helpers... I'm going to try and add the capability to @ember/test-helpers now, but it will require folks upgrade.

This broke embroider's canary tests: embroider-build/embroider#932

@mixonic My work on embroider has been nontrivially impact due to ember-canary repeatedly breaking other core provided libraries such as @ember/test-helpers / @ember/test-waiters. Is it possible to include additional smoke tests here, maybe super-rentals or something? The PR author tends to have all the required context at the time of this PR to quickly fix those issues, if they where aware of the failure.

Anyways, adding at-least @ember/test-helpers or at-best super-rentals to a smoke test for these deprecations merges would really ease the fallout of these otherwise excellent removals.

snewcomer · 2021-08-23T18:28:02Z

@stefanpenner I would be happy to add this. I love adding this type of regression testing!

ember-canary requires `template` + `environment` be provided to the view. This seems like a reasonable change, but does require “low level” libraries such as `@ember/test-helpers` to make some changes. (Ember PR emberjs/ember.js#19680)

stefanpenner · 2021-08-23T18:57:08Z

@snewcomer awesome. At the very-least, I think ember could get lots of milage out of smoke testing @ember/test-helpers.

stefanpenner · 2021-08-23T18:57:26Z

proposed fix for @ember/test-helpers emberjs/ember-test-helpers#1110

stefanpenner · 2021-08-23T18:59:32Z

@snewcomer also, if you have cycles a small inline comment in ember.js, specifically around #19680 (comment) to inform authors that changes need to be made in parallel with @ember/test-helpers would be dope.

Ultimately, having ember provide a "public api" for test-helpers to tap into here could prove to be the best long-term solution. But may not be the most pragmatic choice right now.

description of problem: emberjs/ember.js#19680 (comment) fix in @ember/test-helpers: emberjs/ember-test-helpers#1110

stefanpenner · 2021-08-23T20:49:23Z

Fix for my above issue landed as @ember/test-helpers@2.4.2

* [fixes #932] fix ember-canary test scenario description of problem: emberjs/ember.js#19680 (comment) fix in @ember/test-helpers: emberjs/ember-test-helpers#1110 * [fixes ember-canary/ember-beta] migrate ember-cli-template-lint -> ember-template-lint Ember-cli-template-lint is deprecated, and now incompatible with ember 4.x

snewcomer · 2021-12-01T15:07:38Z

@stefanpenner This PR to smoke test ember-test-helpers, ember-string and ember-legacy-built-in-components is passing now! Would appreciate a review if you have some time.

#19728

snewcomer force-pushed the sn/deprecate-owner-inject branch 2 times, most recently from a11ae84 to bd3db85 Compare July 26, 2021 03:58

mixonic mentioned this pull request Jul 31, 2021

Deprecation Cleanup for 4.0 #19617

Closed

58 tasks

snewcomer force-pushed the sn/deprecate-owner-inject branch 2 times, most recently from fbbd92b to 2a2ea04 Compare August 9, 2021 02:36

snewcomer commented Aug 17, 2021

View reviewed changes

snewcomer force-pushed the sn/deprecate-owner-inject branch from 6a75dd5 to e22a8fa Compare August 17, 2021 14:25

pzuraq approved these changes Aug 17, 2021

View reviewed changes

packages/@ember/-internals/container/lib/registry.ts Outdated Show resolved Hide resolved

packages/@ember/-internals/glimmer/lib/renderer.ts Outdated Show resolved Hide resolved

snewcomer commented Aug 17, 2021

View reviewed changes

snewcomer changed the title ~~Deprecate owner.inject~~ Deprecate and remove owner.inject Aug 17, 2021

snewcomer changed the title ~~Deprecate and remove owner.inject~~ Deprecate owner.inject Aug 17, 2021

mixonic changed the title ~~Deprecate owner.inject~~ [CLEANUP beta] Deprecate owner.inject Aug 18, 2021

mixonic reviewed Aug 18, 2021

View reviewed changes

packages/@ember/-internals/container/lib/registry.ts Outdated Show resolved Hide resolved

snewcomer requested a review from mixonic August 18, 2021 02:29

snewcomer mentioned this pull request Aug 18, 2021

Remove automatic injections willviles/ember-useragent#41

Closed

buschtoens mentioned this pull request Aug 19, 2021

feat: removes auto injection of userAgent service willviles/ember-useragent#42

Merged

mixonic requested changes Aug 20, 2021

View reviewed changes

snewcomer requested a review from mixonic August 20, 2021 02:23

snewcomer mentioned this pull request Aug 20, 2021

[cleanup]: Remove implicit store injection slated for Ember v4.0.0 warp-drive-data/warp-drive#7667

Merged

mixonic approved these changes Aug 22, 2021

View reviewed changes

[CLEANUP beta] Deprecate owner.inject, drop implicit injections

cfad78f

mixonic force-pushed the sn/deprecate-owner-inject branch from 6ed912e to cfad78f Compare August 22, 2021 18:44

mixonic merged commit 955a5e6 into emberjs:master Aug 22, 2021

stefanpenner mentioned this pull request Aug 23, 2021

ember-canary packages/utils appears to be failing embroider-build/embroider#932

Closed

snewcomer deleted the sn/deprecate-owner-inject branch August 23, 2021 18:20

stefanpenner mentioned this pull request Aug 23, 2021

Ensures CoreOutlet is passed the correct parameters (fixes ember-canary) emberjs/ember-test-helpers#1110

Merged

stefanpenner added a commit to embroider-build/embroider that referenced this pull request Aug 23, 2021

[fixes #932] fix ember-canary test scenario

e3a0fe4

description of problem: emberjs/ember.js#19680 (comment) fix in @ember/test-helpers: emberjs/ember-test-helpers#1110

stefanpenner mentioned this pull request Aug 23, 2021

[fixes #932] fix ember-canary test scenario embroider-build/embroider#933

Merged

snewcomer mentioned this pull request Aug 25, 2021

[ci]: @ember/test-helpers partner test #19728

Open

michaelchadwick mentioned this pull request May 8, 2024

fix getOwner deprecation in frontend repo. ilios/ilios#5302

Closed

Uh oh!

Conversation

snewcomer commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mixonic commented Aug 17, 2021

Uh oh!

snewcomer commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzuraq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mixonic commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mixonic left a comment

Choose a reason for hiding this comment

Uh oh!

mixonic commented Aug 22, 2021

Uh oh!

stefanpenner commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snewcomer commented Aug 23, 2021

Uh oh!

stefanpenner commented Aug 23, 2021

Uh oh!

stefanpenner commented Aug 23, 2021

Uh oh!

stefanpenner commented Aug 23, 2021

Uh oh!

stefanpenner commented Aug 23, 2021

Uh oh!

snewcomer commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snewcomer commented Jul 25, 2021 •

edited

Loading

snewcomer commented Aug 17, 2021 •

edited

Loading

mixonic commented Aug 18, 2021 •

edited

Loading

stefanpenner commented Aug 23, 2021 •

edited

Loading