Merge pull request #20376 from emberjs/deprecate-implicit-loading-route

[RFC-774] Deprecate implicit record loading in Ember Route

Author: wagenet

packages/@ember/routing/route.ts

@@ -17,7 +17,7 @@ import { isProxy, lookupDescriptor } from '@ember/-internals/utils';
 import type { AnyFn } from '@ember/-internals/utility-types';
 import Controller from '@ember/controller';
 import type { ControllerQueryParamType } from '@ember/controller';
-import { assert, info, isTesting } from '@ember/debug';
+import { assert, deprecate, info, isTesting } from '@ember/debug';
 import EngineInstance from '@ember/engine/instance';
 import { dependentKeyCompat } from '@ember/object/compat';
 import { once } from '@ember/runloop';
@@ -59,6 +59,16 @@ type RouteTransitionState = TransitionState<Route> & {
 type MaybeParameters<T> = T extends AnyFn ? Parameters<T> : unknown[];
 type MaybeReturnType<T> = T extends AnyFn ? ReturnType<T> : unknown;
 
+interface StoreLike {
+  find(type: string, value: unknown): unknown;
+}
+
+function isStoreLike(store: unknown): store is StoreLike {
+  return (
+    typeof store === 'object' && store !== null && typeof (store as StoreLike).find === 'function'
+  );
+}
+
 export const ROUTE_CONNECTIONS = new WeakMap();
 const RENDER = Symbol('render');
 
@@ -1109,15 +1119,6 @@ class Route<Model = unknown> extends EmberObject.extend(ActionHandler, Evented)
     export default Router;
     ```
 
-    The model for the `post` route is `store.findRecord('post', params.post_id)`.
-
-    By default, if your route has a dynamic segment ending in `_id`:
-
-    * The model class is determined from the segment (`post_id`'s
-      class is `App.Post`)
-    * The find method is called on the model class with the value of
-      the dynamic segment.
-
     Note that for routes with dynamic segments, this hook is not always
     executed. If the route is entered through a transition (e.g. when
     using the `link-to` Handlebars helper or the `transitionTo` method
@@ -1151,12 +1152,21 @@ class Route<Model = unknown> extends EmberObject.extend(ActionHandler, Evented)
     if a promise returned from `model` fails, the error will be
     handled by the `error` hook on `Route`.
 
+    Note that the legacy behavior of automatically defining a model
+    hook when a dynamic segment ending in `_id` is present is
+    [deprecated](https://deprecations.emberjs.com/v5.x#toc_deprecate-implicit-route-model).
+    You should explicitly define a model hook whenever any segments are
+    present.
+
     Example
 
     ```app/routes/post.js
     import Route from '@ember/routing/route';
+    import { service } from '@ember/service';
 
     export default class PostRoute extends Route {
+      @service store;
+
       model(params) {
         return this.store.findRecord('post', params.post_id);
       }
@@ -1233,14 +1243,24 @@ class Route<Model = unknown> extends EmberObject.extend(ActionHandler, Evented)
     @param {Object} value the value passed to find
     @private
   */
-  findModel(...args: any[]) {
-    // SAFETY: it's all absurd lies; there is no explicit contract for `store`
-    // and we allow people to register *anything* here and we call it: GLHF! The
-    // fallback path here means we correctly handle the case where there is no
-    // explicit store injection on the route subclass.
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    let store = ('store' in this ? this.store : get(this, '_store')) as any;
-    return store.find(...args);
+  findModel(type: string, value: unknown) {
+    deprecate(
+      `The implicit model loading behavior for routes is deprecated. ` +
+        `Please define an explicit model hook for ${this.fullRouteName}.`,
+      false,
+      {
+        id: 'deprecate-implicit-route-model',
+        for: 'ember-source',
+        since: { available: '5.3.0' },
+        until: '6.0.0',
+      }
+    );
+
+    const store = 'store' in this ? this.store : get(this, '_store');
+    assert('Expected route to have a store with a find method', isStoreLike(store));
+
+    // SAFETY: We don't actually know it will return this, but this code path is also deprecated.
+    return store.find(type, value) as Model | PromiseLike<Model> | undefined;
   }
 
   /**
@@ -1259,8 +1279,11 @@ class Route<Model = unknown> extends EmberObject.extend(ActionHandler, Evented)
 
     ```app/routes/photos.js
     import Route from '@ember/routing/route';
+    import { service } from '@ember/service';
 
     export default class PhotosRoute extends Route {
+      @service store;
+
       model() {
         return this.store.findAll('photo');
       }
@@ -1564,20 +1587,7 @@ class Route<Model = unknown> extends EmberObject.extend(ActionHandler, Evented)
     return params;
   }
 
-  /**
-    Store property provides a hook for data persistence libraries to inject themselves.
-
-    By default, this store property provides the exact same functionality previously
-    in the model hook.
-
-    Currently, the required interface is:
-
-    `store.find(modelName, findArguments)`
-
-    @property store
-    @type {Object}
-    @private
-  */
+  /** @deprecated Manually define your own store, such as with `@service store` */
   @computed
   protected get _store() {
     const owner = getOwner(this);

packages/@ember/routing/tests/system/route_test.js

@@ -23,7 +23,7 @@ moduleFor(
     }
 
     ['@test default store utilizes the container to acquire the model factory'](assert) {
-      assert.expect(4);
+      assert.expect(5);
 
       let Post = EmberObject.extend();
       let post = {};
@@ -55,12 +55,34 @@ moduleFor(
       let owner = buildOwner(ownerOptions);
       setOwner(route, owner);
 
-      assert.equal(route.model({ post_id: 1 }), post);
-      assert.equal(route.findModel('post', 1), post, '#findModel returns the correct post');
+      expectDeprecation(
+        () =>
+          ignoreAssertion(() => {
+            assert.equal(route.model({ post_id: 1 }), post);
+            assert.equal(route.findModel('post', 1), post, '#findModel returns the correct post');
+          }),
+        /The implicit model loading behavior for routes is deprecated./
+      );
 
       runDestroy(owner);
     }
 
+    ['@test default store can be overridden'](assert) {
+      runDestroy(route);
+
+      let calledFind = false;
+      route = EmberRoute.extend({
+        store: {
+          find() {
+            calledFind = true;
+          },
+        },
+      }).create();
+
+      route.store.find();
+      assert.true(calledFind, 'store.find was called');
+    }
+
     ["@test assert if 'store.find' method is not found"]() {
       runDestroy(route);
 
@@ -77,21 +99,23 @@ moduleFor(
 
       route = owner.lookup('route:index');
 
-      expectAssertion(function () {
-        route.findModel('post', 1);
-      }, `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.`);
+      ignoreDeprecation(() =>
+        expectAssertion(function () {
+          route.findModel('post', 1);
+        }, `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);
     }
@@ -109,14 +133,16 @@ moduleFor(
 
       route = owner.lookup('route:index');
 
-      expectAssertion(function () {
-        route.model({ post_id: 1 });
-      }, `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;\`.`);
+      ignoreDeprecation(() =>
+        expectAssertion(function () {
+          route.model({ post_id: 1 });
+        }, `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);
     }
@@ -130,9 +156,7 @@ moduleFor(
 
       route = owner.lookup('route:index');
 
-      ignoreAssertion(function () {
-        route.model({ post_id: 1 });
-      });
+      ignoreDeprecation(() => ignoreAssertion(() => route.model({ post_id: 1 })));
 
       assert.ok(true, 'no error was raised');
 

packages/ember/tests/routing/decoupled_basic_test.js

@@ -131,17 +131,29 @@ moduleFor(
 
       this.add('model:menu_item', MenuItem);
 
+      let SpecialRoute = class extends Route {
+        model({ menu_item_id }) {
+          return MenuItem.find(menu_item_id);
+        }
+      };
+
+      this.add('route:special', SpecialRoute);
+
       this.addTemplate('special', '<p>{{@model.id}}</p>');
       this.addTemplate('loading', '<p>LOADING!</p>');
 
-      let visited = runTask(() => this.visit('/specials/1'));
-      this.assertText('LOADING!', 'The app is in the loading state');
+      let promise;
+      ignoreDeprecation(() => {
+        let visited = runTask(() => this.visit('/specials/1'));
+        this.assertText('LOADING!', 'The app is in the loading state');
 
-      resolve(menuItem);
+        resolve(menuItem);
 
-      return visited.then(() => {
-        this.assertText('1', 'The app is now in the specials state');
+        promise = visited.then(() => {
+          this.assertText('1', 'The app is now in the specials state');
+        });
       });
+      return promise;
     }
 
     [`@test The loading state doesn't get entered for promises that resolve on the same run loop`](
@@ -161,6 +173,14 @@ moduleFor(
 
       this.add('model:menu_item', MenuItem);
 
+      let SpecialRoute = class extends Route {
+        model({ menu_item_id }) {
+          return MenuItem.find(menu_item_id);
+        }
+      };
+
+      this.add('route:special', SpecialRoute);
+
       this.add(
         'route:loading',
         Route.extend({
@@ -219,7 +239,9 @@ moduleFor(
         })
       );
 
-      runTask(() => handleURLRejectsWith(this, assert, 'specials/1', 'Setup error'));
+      ignoreDeprecation(() => {
+        runTask(() => handleURLRejectsWith(this, assert, 'specials/1', 'Setup error'));
+      });
 
       resolve(menuItem);
     }
@@ -263,6 +285,10 @@ moduleFor(
       this.add(
         'route:special',
         Route.extend({
+          model({ menu_item_id }) {
+            return MenuItem.find(menu_item_id);
+          },
+
           setup() {
             throw new Error('Setup error');
           },

packages/ember/tests/routing/model_loading_test.js

@@ -362,6 +362,14 @@ moduleFor(
       });
       this.add('model:menu_item', MenuItem);
 
+      let SpecialRoute = class extends Route {
+        model({ menu_item_id }) {
+          return MenuItem.find(menu_item_id);
+        }
+      };
+
+      this.add('route:special', SpecialRoute);
+
       this.router.map(function () {
         this.route('home', { path: '/' });
         this.route('special', { path: '/specials/:menu_item_id' });
@@ -390,6 +398,13 @@ moduleFor(
       });
       this.add('model:menu_item', MenuItem);
 
+      let SpecialRoute = class extends Route {
+        model({ menu_item_id }) {
+          return MenuItem.find(menu_item_id);
+        }
+      };
+      this.add('route:special', SpecialRoute);
+
       this.addTemplate('home', '<h3>Home</h3>');
       this.addTemplate('special', '<p>{{@model.id}}</p>');
 
@@ -813,13 +828,20 @@ moduleFor(
     ['@test Route model hook finds the same model as a manual find'](assert) {
       let post;
       let Post = EmberObject.extend();
-      this.add('model:post', Post);
       Post.reopenClass({
         find() {
           post = this;
           return {};
         },
       });
+      this.add('model:post', Post);
+
+      let PostRoute = class extends Route {
+        model({ post_id }) {
+          return Post.find(post_id);
+        }
+      };
+      this.add('route:post', PostRoute);
 
       this.router.map(function () {
         this.route('post', { path: '/post/:post_id' });

tests/docs/expected.js

@@ -484,7 +484,6 @@ module.exports = {
     'sort',
     'sortBy',
     'startRouting',
-    'store',
     'subscribe',
     'sum',
     'tagName',