Implement RFC #523 (@model argument for route templates and {{mount}}) #18345
Conversation
| @@ -138,6 +222,7 @@ function stateFor( | |||
| outlet: render.outlet, | |||
| template, | |||
| controller: render.controller, | |||
| model: render.model, | |||
There was a problem hiding this comment.
Just noting, that changing this structure will likely require changes in @ember/test-helpers:
- https://github.com/emberjs/ember-test-helpers/blob/fdcf31ab04b908e4561eeea102fc07a6f483d640/addon-test-support/%40ember/test-helpers/setup-rendering-context.ts#L115-L141
- https://github.com/emberjs/ember-test-helpers/blob/fdcf31ab04b908e4561eeea102fc07a6f483d640/addon-test-support/ember-test-helpers/legacy-0-6-x/test-module-for-component.js#L227-L239
There was a problem hiding this comment.
model is allowed to be undefined, so it shouldn't cause any errors if it's not present
| @@ -57,6 +57,7 @@ export default class OutletView { | |||
| outlet: TOP_LEVEL_OUTLET, | |||
| name: TOP_LEVEL_NAME, | |||
| controller: undefined, | |||
| model: undefined, | |||
There was a problem hiding this comment.
FWIW, I'd prefer to make this more generic in the internals. As mentioned above, @ember/test-helpers uses outlet state to support rendering tests, and it would be quite nice to allow them to use named arguments (instead of requiring this.foo from the test context). It seems that making this structure a tad more generic (but preventing routes from using it) would allow that.
There was a problem hiding this comment.
seems like a good future improvement 😄
(It is not exactly trivial to translate this to be more generic either, due to the way things are implemented)
There was a problem hiding this comment.
Ya, totally agree this isn't a blocker, but I would love to discuss this sometime with you (hopefully while the issue is still somewhat fresh in your mind).
packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js
Outdated
Show resolved
Hide resolved
|
Most of my comments are around the idea of not testing
|
chancancode
force-pushed
the
model-arg
branch
from
5b088ee
to
08c6021
Compare
Cleaned up a bunch of tests to use `{{this.model}}` explicitly, as opposed to
the implicit `{{model}}`.
Most of these tests just so happened to use a context variable named `model`,
but otherwise has nothing to do with controllers or routing. They probably can
be further refactored to use a different name altogether to avoid confusion.
Also took the opportunity to convert things into async functions in several
places.
This was previously not working properly for `@feature(!SOME_NAME)`.
chancancode
force-pushed
the
model-arg
branch
3 times, most recently
from
eeac160
to
6439f75
Compare
The key changes are in `route.ts` and `syntax/outlet.ts`. The rest are just threading through those new values and test changes.
This is an extension to the RFC not explicitly written in the RFC text.
I missed this when writing the RFC, but we felt that `{{mount}}` with
the `model=` argument is even more clearly an argument, and that we
explicitly decided to restrict the `{{mount}}` syntax to a single
`model` argument (as opposed to arbitrary named arguments), so it is
within the spirit of the RFC to make this work also.
This also refactor the implementation of `{{mount}}` to do less custom
work (like diffing the tag) and let Glimmer VM do more of the work via
the usual paths.
This ensures `{{this.model}}` and the implicit `{{model}}` contunue
to be tested, to prevent any future regressions. It also tests the
behavior described in the RFC where `this.model` and `@model` could
diverge.
chancancode
force-pushed
the
model-arg
branch
from
6439f75
to
59d21e4
Compare
packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js
Show resolved
Hide resolved
This patch implements emberjs/rfcs#523.
See the commit messages for more details.