[BUGFIX 3.28] Improve implicit injections deprecation for routes #19854
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mixonic
force-pushed
the
mixonic/assert-deprecation-on-route-store
branch
from
b7eb388
to
e8b9036
Compare
mixonic
commented
Nov 29, 2021
igorT
reviewed
Nov 29, 2021
|
👍 from me, but someone else should take a look as well |
The store property on routes uses a setter, and so was not impacted by the implicit-injections deprecation issued elsewhere. This may lead app or addon authors to miss usage of a deprecated store injection on the road to 4.0. This improvement of the deprecation fidelity means libraries which use the type injection system to define a store, like Ember Data, should consider another approach to maintain any undeprecated API (for example reopening the route class).
mixonic
force-pushed
the
mixonic/assert-deprecation-on-route-store
branch
from
e8b9036
to
c9e0abe
Compare
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segement, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may expliciting inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implementated and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implementated, and if neither an explict or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Nov 30, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see emberjs#19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0.
rwjblue
approved these changes
Nov 30, 2021
kategengler
pushed a commit
that referenced
this pull request
Dec 1, 2021
When a route has a dynamic segment, several implicit behaviors may be provided by the Ember framework or by the framework in conjunction with a library (commonly Ember Data). * If the route class exists and has a `model(` hook implemented, then that logic is the explicit implementation for data loading on that route. In the far future (say, Ember 5), we want explicit data loading to be the only way data can be loaded. * However, today there are several implicit data loading behaviors. First, the user may explicitly inject a `store` service into the route. If a `model` hook is not implemented, the `find` method on the store will be called. This is, as of this writing, valid in Ember 3 & 4 and not deprecated. * If a `model` hook is not implemented and if an explicit injection is not present, a container/owner-based implicit type or factor injection may have added the `store`. This if valid in Ember 3, but deprecated in Ember 3.28.7 (see #19854), and does not occur in Ember 4 since implicit injections are no longer supported. * If a `model` hook is not implemented, and if neither an explicit or implicit injection of `store` is made, then Ember provides a minimal "default store" implementation. This looks up a model matching the route name, and calls a `find` method on that model. If there is no model, an assertion is thrown. If there a model but it has no `find`, an assertion is thrown. This patch updates those assertions (which are very old) to better coach developers toward the APIs we prefer in modern development. These are: * If you have a route with dynamic segments, you should implement a `model` hook on the route. * Alternatively, and as a transition step for existing code, you may explicitly inject a store on the route to preserve behavior in an application which was relying on implicit injections. There are a lot of edge cases to consider in these copy changes. Many apps use Ember Data. Some apps use different data layers which also provide a `store` service. Some applications implement their own `store` and may implicitly inject it. Some use no `store` at all! Many apps use dynamic segments. Developers may not realize that if a model hook (or route class) has not been defined in Ember and Ember Data 3.28, the `store` service is being relied upon and causes these assertions to never be run. In Ember 3.28 accessing the implicit injection of `store` (and indeed all implicit injection consumption) is deprecated, so existing code should be upgraded to explicit injections and these assertions should never come into play. In Ember 4.0 developers who author: * A new dynamic segment route. * ...with no route class or no model hook. * ...and no explicit service injection. * ...but who also have `model` type factories (likely through Ember Data convention) Would see the second of the updated assertions here. This patch updates the message for 3.28 since it is plausible some non-conventional apps could be seeing this message in 3.x, especially as they upgrade to Ember 4.0. (cherry picked from commit caf4511)
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Dec 2, 2021
The way most Ember apps are setup in late 3.28 (an implicit injection of `store` but also an explicit injection) was not under test. This patch adds a test, and fixes a bug when applications use that pattern. The prior implementation of the `store` injection deprecation on routes (added in emberjs#19854) used a wrapper `DeprecatedStoreInjection` in some cases which was unwrapped by the store getter. A deprecation implemented on `CoreObject` for when a explicit and implicit deprecation mis-match conflicted with that logic. Here, refactor away from the `DeprecatedStoreInjection` wrapper to instead use a WeakSet to track injected instances.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Dec 2, 2021
The way most Ember apps are setup in late 3.28 (an implicit injection of `store` but also an explicit injection) was not under test. This patch adds a test, and fixes a bug when applications use that pattern. The prior implementation of the `store` injection deprecation on routes (added in emberjs#19854) used a wrapper `DeprecatedStoreInjection` in some cases which was unwrapped by the store getter. A deprecation implemented on `CoreObject` for when a explicit and implicit deprecation mis-match conflicted with that logic. Here, refactor away from the `DeprecatedStoreInjection` wrapper to instead use a WeakSet to track injected instances.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Dec 2, 2021
The way most Ember apps are setup in late 3.28 (an implicit injection of `store` but also an explicit injection) was not under test. This patch adds a test, and fixes a bug when applications use that pattern. The prior implementation of the `store` injection deprecation on routes (added in emberjs#19854) used a wrapper `DeprecatedStoreInjection` in some cases which was unwrapped by the store getter. A deprecation implemented on `CoreObject` for when a explicit and implicit deprecation mis-match conflicted with that logic. Here, refactor away from the `DeprecatedStoreInjection` wrapper to instead use a WeakSet to track injected instances.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Dec 2, 2021
The way most Ember apps are setup in late 3.28 (an implicit injection of `store` but also an explicit injection) was not under test. This patch adds a test, and fixes a bug when applications use that pattern. The prior implementation of the `store` injection deprecation on routes (added in emberjs#19854) used a wrapper `DeprecatedStoreInjection` in some cases which was unwrapped by the store getter. A deprecation implemented on `CoreObject` for when a explicit and implicit deprecation mis-match conflicted with that logic. Here, refactor away from the `DeprecatedStoreInjection` wrapper to instead use a WeakSet to track injected instances.
mixonic
added a commit
to mixonic/ember.js
that referenced
this pull request
Dec 2, 2021
The way most Ember apps are setup in late 3.28 (an implicit injection of `store` but also an explicit injection) was not under test. This patch adds a test, and fixes a bug when applications use that pattern. The prior implementation of the `store` injection deprecation on routes (added in emberjs#19854) used a wrapper `DeprecatedStoreInjection` in some cases which was unwrapped by the store getter. A deprecation implemented on `CoreObject` for when a explicit and implicit deprecation mis-match conflicted with that logic. Here, refactor away from the `DeprecatedStoreInjection` wrapper to instead use a WeakSet to track injected instances.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The store property on routes uses a setter, and so was not impacted by the implicit-injections deprecation issued elsewhere. This may lead app or addon authors to miss usage of a deprecated store injection on the road to 4.0.
This improvement of the deprecation fidelity means libraries which use the type injection system to define a store, like Ember Data, should consider another approach to maintain any undeprecated API (for example reopening the route class).
Practically, we're not going to change the use of the injection API in Ember Data 3.28 this late, so this deprecation will log until Ember Data 4.0 is adopted by an app or all codepaths touching the injected store are avoided. With Ember and Ember Data 3.28, that would mean adding explicit model hooks on all routes with dynamic segments.