-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Route inherits parent's model by default. #4246
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
Conversation
|
I think this is correct. You'll just need to wrap in a |
|
Pushed corrections to the points raised. |
|
@locks - Could you change the feature flag to |
|
Done. |
|
lulz |
|
Basically it's gonna take some more convincing to do this for ALL routes, rather than just routes declared via The plan is we'll accept this feature for |
|
I will make the necessary changes tomorrow. |
|
Updated. |
|
@locks - Looks like a rebasing issue. I see 8 commits in this PR... |
|
My bad, PR rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have doubts about this line, talking with @locks on irc about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted, turns out I was doing it backwards. Coupled with a typo, the tests weren't even triggering.
… model by default.
Route inherits parent's model by default.
|
@jonnii et al, with this fix, let us know if/when/how often you have a resource route that doesn't inherit but would be convenient to inherit; as mentioned in an earlier comment, we're only supporting routes (not resources) inheriting parent models, so we're curious how often people really need resources to inherit and why. |
|
@machty the main usecase is for routes, not for resources, so I think limiting it to them is fine. We can delete a bunch of routes that looks like: How does this affect the implicit index route you get when doing |
|
I think it's worth readdressing whether all routes can't simply inherit their parent route's models by default. Now that isn't even really correct anymore. I found this thread because I was wondering why the heck only leaf routes can inherit their parent's model. I definitely have use cases for it elsewhere. Any leaf route can eventually grow the need for children of its own. |
|
@ef4 i think inheriting by default seems correct regardless of depth |

Opening up discussion to address Core f2f proposal: unspecified this.route() models default to parent resource.
Went with the simplest approach I could think of to make it work. All tests passing locally, including the one introduced.
Looking for c+c, thanks in advance :)
Possible improvements:
route