Get handlers lazily #187
Get handlers lazily #187
Conversation
trentmwillis
force-pushed
the
lazy-handlers
branch
from
f11f777
to
82ce899
Compare
|
Unsure why tests are failing. Everything passes in browser and builds fine. Investigating... |
| getHandler: function() {}, | ||
|
|
||
| get handler() { | ||
| if (this.hasOwnProperty('_handler')) { |
There was a problem hiding this comment.
Why do we need the has own check here? Is there a scenario where we have inherited a _handler?
Would be easier/better?
if (this._handler) { return this._handler; }Also, _handler needs to be initialized to undefined or null in the constructor to avoid shaping issues (which I think would make the hasOwnProperty check fail to work since it would have the property but it would be undefined).
There was a problem hiding this comment.
I used hasOwnProperty since it might be undefined, which makes checks tricky. Basically we want "this has been set, but the type of value is ambiguous".
Edit: maybe introduce another boolean to track if it's been set? Feels gross, but otherwise I don't see a clean way of maintaining the shape.
trentmwillis
force-pushed
the
lazy-handlers
branch
from
36f86dc
to
fc945e7
Compare
|
Tests were failing due to an old version of Phantom (1.9.2), bumping |
trentmwillis
force-pushed
the
lazy-handlers
branch
from
8137a01
to
2b538a4
Compare
|
@rwjblue updated this to fix the shaping. |
|
|
||
| getHandler: function() {}, | ||
|
|
||
| _handler: DEFAULT_HANDLER, |
There was a problem hiding this comment.
I think this needs to be in the constructor itself. We want to shape the instance not the prototype...
|
This is good once that last nit-pick is addressed. |
Currently, getHandler is called before constructing a HandlerInfo object. This results in work being done upfront even if the `handler` property of the HandlerInfo is never accessed. This change invokes `getHandler` only when the value is actually needed. This is particularly needed in the case of `getHandler` fetching lazily-loaded routes. Previously lazy routes would be fetched even if the router was just generating a URL; a task which can be done without the whole handler.
trentmwillis
force-pushed
the
lazy-handlers
branch
from
2b538a4
to
0f5289c
Compare
Currently,
getHandleris called before constructing aHandlerInfoobject. Thisresults in work being done upfront even if the
handlerproperty of theHandlerInfois never accessed. This change invokesgetHandleronly when thevalue is actually needed.
This is particularly needed in the case of
getHandlerfetching lazily-loadedroutes. Previously lazy routes would be fetched even if the router was just
generating a URL; a task which can be done without the whole handler.
Note: This change relies on getters/setters which implies IE9+ support (which is in line with Ember's support policy, but I am unsure about this library itself).