-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add deprecation for Route#renderTemplate
#19433
Add deprecation for Route#renderTemplate
#19433
Conversation
063f367
to
d946d29
Compare
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.
Sorry this has been such a hassle @patocallaghan. I think we need to tweak the implementation a bit though. A large number of these tests don't seem to be using a custom renderTemplate
, and they shouldn't be issuing a deprecation (if they do, that would mean that applications basically have no way to avoid the deprecation).
Basically, we need make it so that we avoid deprecation if you don't implement your own renderTemplate
.
@@ -12,6 +12,7 @@ moduleFor( | |||
'Application test: actions', | |||
class extends ApplicationTestCase { | |||
constructor() { | |||
expectDeprecation('Usage of `renderTemplate` is deprecated.'); |
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.
Hmm, I'm a little confused. Why is the deprecation being triggered in this test? I don't see it using renderTemplate
...
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.
So yep this test does fail even though it doesn't have a custom renderTemplate
.
It seems Route#renderTemplate
is used internally by the framework in Route#setup
. This must be why there were so many test failures 🤦♂️ I read the RFC but there was no mention of it so just assumed this was correct.
ember.js/packages/@ember/-internals/routing/lib/system/route.ts
Lines 990 to 992 in 28c18d0
if (this._environment.options.shouldRender) { | |
this.renderTemplate(controller, context); | |
} |
With the fact that the framework relies on this, I'm unsure how to proceed. How can we differentiate between Route#renderTemplate
and a subclassed renderTemplate
? Is it possible?
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.
Yeah, there are a few different ways I think. Off the top of my head we could:
- Change the default implementation to use a private symbol, and only call the actual
renderTemplate
method when present - Detect that the
this.renderTemplate === Route.prototype.renderTemplate
in thedeprecate
invocation
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.
Okay, thanks. I think the second approach is more straightforward so will give that a shot. For the first one could we still have a problem if people call super.renderTemplate(...arguments)
?
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.
Ya, agreed. So the first one is probably not feasible (sorry).
357845d
to
21350a5
Compare
@rwjblue I've updated with your proposed change and yep that made things much simpler. I should have asked for feedback earlier than just assumed all those test failures were accurate 🤦♂️ |
90e8af3
to
2f6e493
Compare
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.
Thank you @patocallaghan!
Adds deprecation for
Route#renderTemplate
. See RFC 418 for more info.I'm not going to lie, this was a bit rough with the amount of failing tests which needed to be fixed and then a few rebases in between 😅