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

Deprecate implicit record loading in routes #557

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions text/0000-deprecate-implict-model-finding-in-routes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
- Start Date: 2019-11-27
- Relevant Team(s): Ember.js, Ember Data, Learning
- RFC PR: https://github.com/emberjs/rfcs/pull/557/files
- Tracking: (leave this empty)

# Deprecate Implicit Record Finding In Routes

## Summary

Today, Ember's Route's `model` hook has an implicit convention based on the name and casing of a dynamic segment / parameter passed to the model hook.
For example, if a dynamic segment is `:post_id`, there exists logic to split on the underscore, look up a service on the route named `store`, and find a record of type `post`. This is far from the explicit and discoverable data loading path we teach today and deprecating this code for removal will help align with our current easier-to-teach/explict data loading patterns.

## Motivation

Recently, [eslint-plugin-ember](https://github.com/ember-cli/eslint-plugin-ember/issues/410) introduced a lint rule to _enforce_ the `snake_case` convention of dynamic segments. I and a few others don't like `snake_case` in JavaScript, as the convention is `camelCase`. We looked into submitting an RFC for changing the default to `camelCase`, but encountered [this code](https://github.com/emberjs/ember.js/blob/b4456779d0f5f5b99da853c4e1f0688ce96cc27c/packages/%40ember/-internals/routing/lib/system/route.ts#L1167-L1186) that suggests some implicit data loading behavior for `snake_case` params _ending_ in `_id` and if you _happen_ to have a service named `store`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, it isn't just when you have service named store. Ember itself has a default this.store property on @ember/routing/route:

https://github.com/emberjs/ember.js/blob/b4456779d0f5f5b99da853c4e1f0688ce96cc27c/packages/%40ember/-internals/routing/lib/system/route.ts#L2220

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this link:

https://github.com/emberjs/ember.js/blob/b4456779d0f5f5b99da853c4e1f0688ce96cc27c/packages/%40ember/-internals/routing/lib/system/route.ts#L1167-L1186

Is actually showing two different issues:

This RFC is (AFAICT) only talking about deprecating the first thing and not the second.


## Transition Path

We don't teach this behavior, so the transition path should be fairly light.

if someone _doesn't_ define a `model` hook on their route, and are taking advantage of the implicit record loading behavior, they will be presented with a deprecation warning in the console [triggered from here](https://github.com/emberjs/ember.js/blob/b4456779d0f5f5b99da853c4e1f0688ce96cc27c/packages/%40ember/-internals/routing/lib/system/route.ts#L1209) -- showing that removal will happen in Ember v4.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a bit more details.

  • How do you determine that they are taking advantage of the implicit record loading behaviors? Is it only when that this.findModel invocation actually returns a value?
  • We should also deprecate using the implicit this.store on @ember/routing/route (the one here). This would not affect ember-data users because ember-data clobbers with an initializer injection on all routes, or users already leveraging @service store in their Route file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit injections have now been removed for 4.0!

#680


A person currently using this implicit behavior should add to their route:

```ts
async model({ post_id }) {
return this.store.find('post', post_id);
}
```

and if a route doesn't exist in the app, it will need to be defined.

## How We Teach This

Ember Guides shouldn't need updating.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the guides will need to be changed. The auto model loading behavior is mentioned in the Dynamic segments section.


Adding an entry to the deprecation guides should be sufficient, briefly explaining the before/after from above should be sufficient.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the content of this section with the deprecation guide entry contents? IMHO this is really the main part of this particular RFC and being able to discuss the actual suggestions / prose would be really useful.


## Drawbacks

Drawbacks only exist for people who are utilizing this implicit behavior. For them, the drawbacks are:
- they now have to define a model hook
- they may have to create a route file where previously the route was implicitly defined for them and implicitly loading the record for them as well
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this isn't a drawback 😉


## Alternatives

Impact of not doing this: stale code, added weight for everyone else who isn't using this feature, or who isn't using ember-data.

## Unresolved questions

None at this time.