Skip to content

Commit

Permalink
[BUGFIX release] Improve assert in default store
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mixonic committed Nov 30, 2021
1 parent b88bd24 commit e4146db
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 10 deletions.
29 changes: 23 additions & 6 deletions packages/@ember/-internals/routing/lib/system/route.ts
Expand Up @@ -25,7 +25,6 @@ import Controller from '@ember/controller';
import { assert, info, isTesting } from '@ember/debug';
import { dependentKeyCompat } from '@ember/object/compat';
import { once } from '@ember/runloop';
import { classify } from '@ember/string';
import { DEBUG } from '@glimmer/env';
import { Template, TemplateFactory } from '@glimmer/interfaces';
import {
Expand Down Expand Up @@ -1736,16 +1735,18 @@ class Route extends EmberObject.extend(ActionHandler, Evented) implements IRoute
protected get store() {
let owner = getOwner(this);
let routeName = this.routeName;
let namespace = get(this, '_router.namespace');

return {
find(name: string, value: unknown) {
let modelClass: any = owner.factoryFor(`model:${name}`);

assert(
`You used the dynamic segment ${name}_id in your route ${routeName}, but ${namespace}.${classify(
name
)} did not exist and you did not override your route's \`model\` hook.`,
`You used the dynamic segment \`${name}_id\` in your route ` +
`\`${routeName}\` for which Ember requires you provide a ` +
`data-loading implementation. Commonly, that is done by ` +
`adding a model hook implementation on the route ` +
`(\`model({${name}_id}) {\`) or by injecting an implemention of ` +
`a data store: \`@service store;\`.`,
Boolean(modelClass)
);

Expand All @@ -1755,7 +1756,22 @@ class Route extends EmberObject.extend(ActionHandler, Evented) implements IRoute

modelClass = modelClass.class;

assert(`${classify(name)} has no method \`find\`.`, typeof modelClass.find === 'function');
assert(
`You used the dynamic segment \`${name}_id\` in your route ` +
`\`${routeName}\` for which Ember requires you provide a ` +
`data-loading implementation. Commonly, that is done by ` +
`adding a model hook implementation on the route ` +
`(\`model({${name}_id}) {\`) or by injecting an implemention of ` +
`a data store: \`@service store;\`.\n\n` +
`Rarely, applications may attempt to use a legacy behavior where ` +
`the model class (in this case \`${name}\`) is resolved and the ` +
`\`find\` method on that class is invoked to load data. In this ` +
`application, a model of \`${name}\` was found but it did not ` +
`provide a \`find\` method. You should not add a \`find\` ` +
`method to your model. Instead, please implement an appropriate ` +
`\`model\` hook on the \`${routeName}\` route.`,
typeof modelClass.find === 'function'
);

return modelClass.find(value);
},
Expand Down Expand Up @@ -2456,3 +2472,4 @@ Route.reopen({
});

export default Route;
export default Route;
35 changes: 31 additions & 4 deletions packages/@ember/-internals/routing/tests/system/route_test.js
Expand Up @@ -70,14 +70,31 @@ moduleFor(
let owner = buildOwner();
let Post = EmberObject.extend();

owner.register('route:index', EmberRoute);
owner.register(
'route:index',
EmberRoute.extend({
routeName: 'index',
})
);
owner.register('model:post', Post);

route = owner.lookup('route:index');

expectAssertion(function () {
route.findModel('post', 1);
}, 'Post has no method `find`.');
}, `You used the dynamic segment \`post_id\` in your route ` +
`\`index\` for which Ember requires you provide a ` +
`data-loading implementation. Commonly, that is done by ` +
`adding a model hook implementation on the route ` +
`(\`model({post_id}) {\`) or by injecting an implemention of ` +
`a data store: \`@service store;\`.\n\n` +
`Rarely, applications may attempt to use a legacy behavior where ` +
`the model class (in this case \`post\`) is resolved and the ` +
`\`find\` method on that class is invoked to load data. In this ` +
`application, a model of \`post\` was found but it did not ` +
`provide a \`find\` method. You should not add a \`find\` ` +
`method to your model. Instead, please implement an appropriate ` +
`\`model\` hook on the \`index\` route.`);

runDestroy(owner);
}
Expand All @@ -86,13 +103,23 @@ moduleFor(
runDestroy(route);

let owner = buildOwner();
owner.register('route:index', EmberRoute);
owner.register(
'route:index',
EmberRoute.extend({
routeName: 'index',
})
);

route = owner.lookup('route:index');

expectAssertion(function () {
route.model({ post_id: 1 });
}, /You used the dynamic segment post_id in your route undefined, but <.*:ember\d+>.Post did not exist and you did not override your route's `model` hook./);
}, `You used the dynamic segment \`post_id\` in your route ` +
`\`index\` for which Ember requires you provide a ` +
`data-loading implementation. Commonly, that is done by ` +
`adding a model hook implementation on the route ` +
`(\`model({post_id}) {\`) or by injecting an implemention of ` +
`a data store: \`@service store;\`.`);

runDestroy(owner);
}
Expand Down

0 comments on commit e4146db

Please sign in to comment.