-
-
Notifications
You must be signed in to change notification settings - Fork 408
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 Ember Route #774
Conversation
d1c1eee
to
fc0f0f1
Compare
@scottmessinger @runspired is there anything more that we need here other than an "official" review? |
@wagenet this is a framework team item not a data team item though Scott did pick it up from @NullVoxPopuli it seems to try to get it over the line. I think this became of increased importance because the troll behavior lead to increased bug reports and confusion once injections were eliminated for 4.x |
ah right! this supersedes #557 <-- I'll close this |
100%. I’d be eager to do the legwork once merged if someone else didn’t pick it up. Unsure if it needs a “sponsor”. Let me know if someone can bring it up in the core team meeting! |
Framework core discussed this and everyone is in favor of the deprecation. A suggestion is that the design could use an optional feature that disables the old bad behavior. This would let people clear the deprecation by enabling the feature, rather than requiring them to create new empty model hooks and potentially new route files. |
Co-authored-by: Bert De Block <[email protected]>
Co-authored-by: Bert De Block <[email protected]>
Co-authored-by: Bert De Block <[email protected]>
Co-authored-by: Bert De Block <[email protected]>
d9547c5
to
4419a8f
Compare
@ef4 hi! Any updates on this RFC? |
@snewcomer looks like it may have gotten dropped on accident, sorry! I've officially added it to the agenda for our next meeting. |
@snewcomer Sorry for the delay. Everything got thrown off with the holidays. I'll try to get this back on track. |
We agreed to move this to FCP. It needs a few tweaks for our new stages setup and will need to be retargeted at v6. I can help with this. |
Relatedly, we should also deprecate the default store property in routes. |
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.
Looks good! In addition to the inline comments, can you add a "Drawbacks" section? I think it's sufficient to just note that when the implicit behavior is nice it saves typing and now you have to type more.
I think this also means that the I've been disabling that rule on every project I work on since we don't depend on the implicit record loading and it would be nice if we could remove it from our eslint config file 😄. |
Co-authored-by: Peter Wagenet <[email protected]>
Co-authored-by: Peter Wagenet <[email protected]>
Advance RFC #774 `"Deprecate Implicit Record Loading in Routes"` to Stage Released
Advance RFC #774 `"Deprecate Implicit Record Loading in Routes"` to Stage Recommended
Rendered
close #773
close #557