Deprecate implicit record loading in routes #557

NullVoxPopuli · 2019-11-27T14:51:04Z

runspired · 2019-11-27T17:40:41Z

This behavior and implicit store are unrelated to EmberData. EmberData implements the find method and injects itself eagerly in order to correct the behavior of the default for users of EmberData, but if you are not using EmberData you will still have a store and this auto find behavior.

This RFC should likely deprecate defaultStore and it’s behaviors more widely.

NullVoxPopuli · 2019-11-27T17:43:01Z

, but if you are not using EmberData you will still have a store and this auto find behavior.

what is loaded? I've never heard of defaultStore before today.

runspired · 2019-11-27T17:56:06Z

https://github.com/emberjs/ember.js/blob/03ad75925a58080ac1dfac25d7e2dc81214adc8a/packages/ember-routing/lib/system/route.js#L1653

@NullVoxPopuli

NullVoxPopuli · 2019-11-27T17:58:31Z

oof. ok. thanks. yeah, this deprecation should def include deprecating that as well :D

joukevandermaas · 2019-12-05T12:43:50Z

BIG fan of this RFC. A few weeks ago I spent half a day trying to figure out why our app was doing an unexpected request, until I stumbled upon this gem of functionality 😅.

Yes, the behavior is fun if you know about it, but if you don't it can be very confusing.

Typo

runspired · 2019-12-06T04:39:53Z

@NullVoxPopuli are you going to update this RFC to eliminate defaultStore?

Also for the alternatives I would note that defaultStore negatively impacts users of EmberData as well, and forces EmberData to do hacky things that we don't want to do. We have wished for this to be deprecated and removed for years. Thank you for taking it on <3

tniezurawski · 2019-12-06T07:31:18Z

Removing a bit of magic that isn't explained anywhere sounds good 👍

I'm not sure about the Motivation section and enforcing the case on params/variables. I believe that's not a subject of this RFC and should stay in devs' lint configs. IMO the true motivation is to remove that implicit record loading. Although I cringe every time I have to use something else than camelCase I think we shouldn't force anything.

rwjblue · 2019-12-06T11:20:47Z

Although I cringe every time I have to use something else than camelCase I think we shouldn't force anything.

Yes, I definitely agree with this point.

dcyriller · 2020-02-23T19:25:48Z

For reference, the functionality (default store + implicit record loading) is explained in this article: https://blog.pablobm.com/2019/01/15/store-without-ember-data.html

rwjblue · 2020-05-20T14:33:25Z

text/0000-deprecate-implict-model-finding-in-routes.md

+
+## 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`.


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

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:

Implicit :post_id model lookup (which is what this RFC is talking about)

Implicit "parent model" (see Route inherits parent's model by default. ember.js#4246 for where that was introduced)

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

rwjblue · 2020-05-20T14:39:03Z

text/0000-deprecate-implict-model-finding-in-routes.md

+
+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.


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.

Implicit injections have now been removed for 4.0!

#680

rwjblue · 2020-05-20T14:40:06Z

text/0000-deprecate-implict-model-finding-in-routes.md

+
+Ember Guides shouldn't need updating.
+
+Adding an entry to the deprecation guides should be sufficient, briefly explaining the before/after from above should be sufficient.


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.

rwjblue · 2020-05-20T14:40:32Z

text/0000-deprecate-implict-model-finding-in-routes.md

+
+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


IMHO this isn't a drawback 😉

jelhan · 2020-08-08T11:11:28Z

Moving this forward would be great. The bugs caused by this magic are hard to debug and very confusing to people new to ember.

It so easy to miss defining a model hook. It's very likely that developer notice what's going wrong when dealing with a this.model or @model, which is undefined. But due to this magic often an error is thrown, which causes confusion and pulls the developer on a wrong track.

Maybe that should be mentioned also explicitly in the motivation section.

Windvis · 2021-02-23T16:03:26Z

text/0000-deprecate-implict-model-finding-in-routes.md

+
+## How We Teach This
+
+Ember Guides shouldn't need updating.


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

wagenet · 2022-07-23T23:48:09Z

@NullVoxPopuli where does this stand?

NullVoxPopuli · 2022-07-24T00:02:11Z

I don't know if it's worth pursuing if our next routing system is just.. different entirely 🙃

maybe we/i/someone need to spike out what that next routing system would be like (actual usage / code, rather than readme-driven development), and see if there is a way to integrate that into the current system, and then maybe route-by-route opt in to it, and eventually deprecate the old thing.

Having interop between routers could be interesting tho -- need to look in to where the constraints are.

runspired · 2022-07-24T00:07:59Z

I think it's heavily worth pursuing, if for no other reason it removes one more crazy piece that the next system would have to account for the transition for. We know we don't want this behavior, may as well discard it now.

NullVoxPopuli · 2022-07-24T00:12:22Z

excellent -- well if anyone wants to help out with this RFC, applying the above feedback, that'd be super :D

NullVoxPopuli · 2022-07-24T00:16:41Z

Superseded by: #774

NullVoxPopuli added 2 commits November 27, 2019 06:50

Deprecate implicit record loading in routes

e0202b2

add RFC link

28d28c2

This was referenced Nov 27, 2019

Intent to RFC: make the default router dynamic segments camelCase. #556

Closed

Deprecate defaultStore located at Route.store #377

Closed

Bouke and others added 2 commits December 5, 2019 18:22

Typo

9bca793

Merge pull request #10 from Bouke/patch-1

9b4410a

Typo

rwjblue requested changes May 20, 2020

View reviewed changes

rwjblue added the T-framework RFCs that impact the ember.js library label May 20, 2020

rwjblue self-assigned this May 20, 2020

snewcomer mentioned this pull request Nov 6, 2020

Deprecate Implicit Injection on arbitrary Ember Framework objects #680

Merged

Windvis reviewed Feb 23, 2021

View reviewed changes

snewcomer mentioned this pull request Nov 14, 2021

Deprecate implicit record loading in Ember Route #774

Merged

NullVoxPopuli closed this Jul 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate implicit record loading in routes #557

Deprecate implicit record loading in routes #557

NullVoxPopuli commented Nov 27, 2019 •

edited

Loading

runspired commented Nov 27, 2019

NullVoxPopuli commented Nov 27, 2019

runspired commented Nov 27, 2019

NullVoxPopuli commented Nov 27, 2019

joukevandermaas commented Dec 5, 2019

runspired commented Dec 6, 2019

tniezurawski commented Dec 6, 2019

rwjblue commented Dec 6, 2019

dcyriller commented Feb 23, 2020

rwjblue May 20, 2020

rwjblue May 20, 2020

rwjblue May 20, 2020

snewcomer Nov 13, 2021

rwjblue May 20, 2020

rwjblue May 20, 2020

jelhan commented Aug 8, 2020 •

edited

Loading

Windvis Feb 23, 2021

wagenet commented Jul 23, 2022

NullVoxPopuli commented Jul 24, 2022

runspired commented Jul 24, 2022

NullVoxPopuli commented Jul 24, 2022

NullVoxPopuli commented Jul 24, 2022


		## 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`.


		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.


		Ember Guides shouldn't need updating.

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

Deprecate implicit record loading in routes #557

Deprecate implicit record loading in routes #557

Conversation

NullVoxPopuli commented Nov 27, 2019 • edited Loading

runspired commented Nov 27, 2019

NullVoxPopuli commented Nov 27, 2019

runspired commented Nov 27, 2019

NullVoxPopuli commented Nov 27, 2019

joukevandermaas commented Dec 5, 2019

runspired commented Dec 6, 2019

tniezurawski commented Dec 6, 2019

rwjblue commented Dec 6, 2019

dcyriller commented Feb 23, 2020

rwjblue May 20, 2020

Choose a reason for hiding this comment

rwjblue May 20, 2020

Choose a reason for hiding this comment

rwjblue May 20, 2020

Choose a reason for hiding this comment

snewcomer Nov 13, 2021

Choose a reason for hiding this comment

rwjblue May 20, 2020

Choose a reason for hiding this comment

rwjblue May 20, 2020

Choose a reason for hiding this comment

jelhan commented Aug 8, 2020 • edited Loading

Windvis Feb 23, 2021

Choose a reason for hiding this comment

wagenet commented Jul 23, 2022

NullVoxPopuli commented Jul 24, 2022

runspired commented Jul 24, 2022

NullVoxPopuli commented Jul 24, 2022

NullVoxPopuli commented Jul 24, 2022

NullVoxPopuli commented Nov 27, 2019 •

edited

Loading

jelhan commented Aug 8, 2020 •

edited

Loading