From caf451172df5306a351023b9bb4a5d0f01e76792 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Tue, 30 Nov 2021 09:36:01 -0500 Subject: [PATCH] [BUGFIX release] Improve assert in default store 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 https://github.com/emberjs/ember.js/pull/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. --- .../-internals/routing/lib/system/route.ts | 28 +++++++++++---- .../routing/tests/system/route_test.js | 35 ++++++++++++++++--- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/@ember/-internals/routing/lib/system/route.ts b/packages/@ember/-internals/routing/lib/system/route.ts index 277ff633b40..0ae59b37d5b 100644 --- a/packages/@ember/-internals/routing/lib/system/route.ts +++ b/packages/@ember/-internals/routing/lib/system/route.ts @@ -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 { @@ -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) ); @@ -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); }, diff --git a/packages/@ember/-internals/routing/tests/system/route_test.js b/packages/@ember/-internals/routing/tests/system/route_test.js index fbc5de2c80d..2b6fd159f29 100644 --- a/packages/@ember/-internals/routing/tests/system/route_test.js +++ b/packages/@ember/-internals/routing/tests/system/route_test.js @@ -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); } @@ -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); }