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

[BUGFIX release] Improve assert in default store #19858

Merged
merged 1 commit into from Nov 30, 2021

Conversation

mixonic
Copy link
Sponsor Member

@mixonic mixonic commented Nov 30, 2021

Mirrors #19857, but for Ember 4.0+.

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 [BUGFIX 3.28] Improve implicit injections deprecation for routes #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.

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 mixonic force-pushed the mixonic/update-assertion-language-4 branch from e4146db to caf4511 Compare November 30, 2021 15:59
@mixonic mixonic marked this pull request as ready for review November 30, 2021 16:00
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 30, 2021
When generating a route with a dynamic segment, say via:

    ember g route foo --path="bar/:buzz_id"

The default empty route definition will cause an awkward assertion to be
thrown.

* In 3.28 without any data layer, the user is prompted via assertion to
  implement a model hook.
* In 3.28 with Ember Data, an implicit fetch via Ember Data happens.
* In 4.0 without any data layer, the user would be prompted via
  assertion to implement a model hook.
* In 4.0 with Ember Data, the user would be prompted via assertion to
  either add a `find` method (old assertion) or to implement a model
  hook (new assertion via
  emberjs#19858).

It is doubtless that many users will still encounter these behaviors,
but updating the blueprints to generate a model hook by default improves
on the happy path.

In theory this could do back to 3.28, however the value there is
somewhat less since Ember Data's implicit store injection remains in
that version (and therefore the assertions/messages are less confusing).
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 30, 2021
When generating a route with a dynamic segment, say via:

    ember g route foo --path="bar/:buzz_id"

The default empty route definition will cause an awkward assertion to be
thrown.

* In 3.28 without any data layer, the user is prompted via assertion to
  implement a model hook.
* In 3.28 with Ember Data, an implicit fetch via Ember Data happens.
* In 4.0 without any data layer, the user would be prompted via
  assertion to implement a model hook.
* In 4.0 with Ember Data, the user would be prompted via assertion to
  either add a `find` method (old assertion) or to implement a model
  hook (new assertion via
  emberjs#19858).

It is doubtless that many users will still encounter these behaviors,
but updating the blueprints to generate a model hook by default improves
on the happy path.

In theory this could do back to 3.28, however the value there is
somewhat less since Ember Data's implicit store injection remains in
that version (and therefore the assertions/messages are less confusing).
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 30, 2021
When generating a route with a dynamic segment, say via:

    ember g route foo --path="bar/:buzz_id"

The default empty route definition will cause an awkward assertion to be
thrown.

* In 3.28 without any data layer, the user is prompted via assertion to
  implement a model hook.
* In 3.28 with Ember Data, an implicit fetch via Ember Data happens.
* In 4.0 without any data layer, the user would be prompted via
  assertion to implement a model hook.
* In 4.0 with Ember Data, the user would be prompted via assertion to
  either add a `find` method (old assertion) or to implement a model
  hook (new assertion via
  emberjs#19858).

It is doubtless that many users will still encounter these behaviors,
but updating the blueprints to generate a model hook by default improves
on the happy path.

In theory this could do back to 3.28, however the value there is
somewhat less since Ember Data's implicit store injection remains in
that version (and therefore the assertions/messages are less confusing).
@rwjblue rwjblue merged commit e317a88 into emberjs:master Nov 30, 2021
@rwjblue rwjblue deleted the mixonic/update-assertion-language-4 branch November 30, 2021 19:39
kategengler pushed a commit that referenced this pull request Dec 1, 2021
When generating a route with a dynamic segment, say via:

    ember g route foo --path="bar/:buzz_id"

The default empty route definition will cause an awkward assertion to be
thrown.

* In 3.28 without any data layer, the user is prompted via assertion to
  implement a model hook.
* In 3.28 with Ember Data, an implicit fetch via Ember Data happens.
* In 4.0 without any data layer, the user would be prompted via
  assertion to implement a model hook.
* In 4.0 with Ember Data, the user would be prompted via assertion to
  either add a `find` method (old assertion) or to implement a model
  hook (new assertion via
  #19858).

It is doubtless that many users will still encounter these behaviors,
but updating the blueprints to generate a model hook by default improves
on the happy path.

In theory this could do back to 3.28, however the value there is
somewhat less since Ember Data's implicit store injection remains in
that version (and therefore the assertions/messages are less confusing).

(cherry picked from commit e253e92)
@mixonic mixonic mentioned this pull request Dec 14, 2021
19 tasks
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

2 participants