Skip to content

Conversation

@locks
Copy link
Contributor

@locks locks commented Jan 27, 2014

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:

  • Test nested route
  • Test more than one level of nesting
  • features.json
  • FEATURES.md
  • prefix commit
  • only leaf route/resources inherit model

@machty
Copy link
Contributor

machty commented Jan 27, 2014

I think this is correct. You'll just need to wrap in a ember-route-inherits-parent-model flag and prefix commit with [FEATURE ember-route-inherits-parent-model]

@locks
Copy link
Contributor Author

locks commented Jan 27, 2014

Pushed corrections to the points raised.

@rwjblue
Copy link
Member

rwjblue commented Jan 27, 2014

@locks - Could you change the feature flag to ember-routing-inherits-parent-model? Generally, we like the prefix to match the package that it is modifying.

@locks
Copy link
Contributor Author

locks commented Jan 27, 2014

Done.

@jonnii
Copy link

jonnii commented Jan 28, 2014

This is how I'll feel once this get merged and I can delete a bunch of routes:

ibcns4kmb06ia9

@ebryn
Copy link
Member

ebryn commented Jan 30, 2014

lulz

@wagenet
Copy link
Member

wagenet commented Feb 3, 2014

@machty, @ebryn Did this get discussed in a core team meeting? If not, we should probably give it a quick Go or No-Go before we merge.

@machty
Copy link
Contributor

machty commented Feb 3, 2014

Basically it's gonna take some more convincing to do this for ALL routes, rather than just routes declared via this.route().

The plan is we'll accept this feature for route routes and not resource routes. @locks if you wanna make the change, I believe you can simply add a check to only perform this behavior if resolveIndex === handlerInfos.length-1. Once that's in we'll approve this, and in the future we may add universal support (to include this.resource routes), but we can save that discussion for later.

@locks
Copy link
Contributor Author

locks commented Feb 3, 2014

I will make the necessary changes tomorrow.

@locks
Copy link
Contributor Author

locks commented Feb 4, 2014

Updated.

@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2014

@locks - Looks like a rebasing issue. I see 8 commits in this PR...

@locks
Copy link
Contributor Author

locks commented Feb 4, 2014

My bad, PR rebased.

Copy link
Contributor

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

Copy link
Contributor Author

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.

machty added a commit that referenced this pull request Feb 4, 2014
Route inherits parent's model by default.
@machty machty merged commit c0ce02c into emberjs:master Feb 4, 2014
@machty
Copy link
Contributor

machty commented Feb 4, 2014

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

@jonnii
Copy link

jonnii commented Feb 4, 2014

@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:

App.ZomgEditRoute = App.AuthenticatedRoute.extend
  model: ->
    @modelFor 'zomg'

How does this affect the implicit index route you get when doing this.resource('zomg', function(){})?

@ef4
Copy link
Contributor

ef4 commented Sep 19, 2014

I think it's worth readdressing whether all routes can't simply inherit their parent route's models by default. Now that route supports nested children, the existing check:

if (transition.resolveIndex !== transition.state.handlerInfos.length-1) { return; }

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.

@stefanpenner
Copy link
Member

@ef4 i think inheriting by default seems correct regardless of depth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants