-
-
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
Refactor route to lookup controller for QPs. #15178
Conversation
Previously, the `_qp` computed property attempted to access properties off of the `definedControllerClass.proto()`. We even specifically intended to support accessing other computed properties or injected services to share query param values on. Unfortunately, this entire system relied on the old (and deprecated) double extend that was done by `lookupFactory` (because CPs and service injections all may need access to injections like `owner`). When `ember-no-double-extend` was introduced a "backdoor" was added to allow `Route#_qp` to use a non-deprecated version of `.lookupFactory`. The original version of `_qp` used `ResolvedController.proto()` as a performance optimization (to prevent extra instantiation and whatnot), but at this point using the legacy double extend mechanism and relying on `.proto()` is causing more trouble than its worth. This change, makes accessing `_qp` instantiate the controller instances (without the costly double extend), and simply accesses values on them. Ultimately, this will force controllers to be instantiated earlier than previously (though there were no guarantees on _when_ they would be instantiated). This instantiation _should_ be cheaper than forcing the double extend for these controllers, and it makes `_qp` much less brittle since it is now using a standard public API.
I did a quick run of this with ember-macro-benchmark (which does use QPs but on a fairly small scale). Raw results: |
I'd like to let this ride the next beta train. IMO it makes things much less brittle allows us to avoid forced double extend scenarios for folks with QPs.... |
Ya i suspect this was just wrong, the larger of the performance issues was actually loading the code, not instantiating the it. |
Your call, I personally think this is fine to 🚢 but if your not comfortable with that that is fine. |
Ya, I was meaning that merge to master for the 2.14 beta cycle (not try to rush it into 2.13 just before release). |
Previously, the
_qp
computed property attempted to access properties off of thedefinedControllerClass.proto()
. We even specifically intended to support accessing other computed properties or injected services to share query param values on. Unfortunately, this entire system relied on the old (and deprecated) double extend that was done bylookupFactory
(because CPs and service injections all may need access to injections likeowner
). Whenember-no-double-extend
was introduced a "backdoor" was added to allowRoute#_qp
to use a non-deprecated version of.lookupFactory
.The original version of
_qp
usedResolvedController.proto()
for two reasons:.proto()
is causing more trouble (and likely more costly) than its worth.model
was, we would instantiate a different controller base class). The controller base class no longer changes based on themodel
hook of the route (and hasn't for all of 2.0.0), so that concern is moot.This change, makes accessing
_qp
instantiate the controller instances (without the costly double extend), and simply accesses values on them. Ultimately, this will force controllers to be instantiated earlier than previously (though there were no guarantees on when they would be instantiated). This instantiation should be cheaper than forcing the double extend for these controllers, and it makes_qp
much less brittle since it is now using a standard public API.