diff --git a/text/0000-non-context-shifting-partials.md b/text/0000-non-context-shifting-partials.md new file mode 100644 index 0000000000..f9e8053eba --- /dev/null +++ b/text/0000-non-context-shifting-partials.md @@ -0,0 +1,351 @@ +- Start Date: 2017-10-27 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +Introduce a new form of `{{partial}}` without the "context-shifting" +behavior (automatically getting access to any variables available to the +calling context) and deprecate the current form. + +```hbs +{{#each model as |post|}} + {{#with post.canonicalUrl as |url|}} + {{!-- The usual form of partial will automatically inherit all the + variables in scope (such as `post` here) plus everything else on + `this` (such as `model`) --}} + {{partial "post-header"}} + + {{#each post.authors as |author|}} + {{!-- This partial will only have access to the explicitly passed + `post` and `author` (via the name `contributor`) variables, but not + `url`, `model` and everything else on `this` --}} + {{partial "contributor-card" post=post contirbutor=author}} + {{/each}} + {{/with}} +{{/each}} +``` + +# Motivation + +In its current form, `{{partial}}` is the only remaining "context-shifting +helper" from the pre-1.0 era. That is, the "context" (what variables are +available to the partial) depends on the calling context. This makes partials +incredibly difficult to understand and is a refactoring hazard. + +For example, you might scan at a template and decide a variable (either +defined on the controller/component, or introduced in the template via a +block param) is unused and can be safely removed. However, that assumption +might not hold if there are one or more `{{partial}}` invocations in the +template. + +This is contrary to components – where their invocation requires you to pass +in any data explicitly, in the form of arguments. Not only does this make it +possible to freely refactor the calling template (as long as you don't change +or remove the passed arguments, the component should continue to work), it +also offers some hints about what the component might do, as opposed to partials +where its name would be the only clue. + +Drawing parallel to JavaScript code, if component invocations were function +calls, then partial invocations are `eval`: + +```js +class MyTemplate { + // ... + render() { + let { model, name } = this; + + MyForm(f => { + // Invoking a component is similar to calling a function. You can + // directly see what data are being passed and feel confident about + // any refactor that does not change said data (such as replacing + // `model: this.model` with just `model` and renaming the outer + // `name` variable to make clear it is unrelated to the `name` being + // passed here. + AnotherComponent({ f, model: this.model, name: "Godfrey" }); + + // Invoking a partial, on the other hand, is similar to an `eval`. + // You cannot tell what variables are being used inside the without + // carefully reading its source, so any refactor here (such as + // removing the unused `model` and `name` destructure at the top) + // will likely break the code. + eval(AnotherPartial); + }); + } +} +``` + +Not only does this make partials difficult for humans to understand, all of +these problems apply to machines as well. In fact, the "function" vs "eval" +analogy is more than just a illustration of how to reason about their difference, +it actually closely matches the way these two features are modelled/implemented +in the Glimmer VM. This makes it really complicated and [error-prone][partial-bug] +to implement `{{partial}}`. + +[partial-bug]: https://github.com/glimmerjs/glimmer-vm/pull/707 + +Futhermore, the extra book keeping to enable what amounts to an `eval()` also +means they are much slower than regular components. (The opposite used to be true +so users who were used to using partials as a performance escape-hatch are +probably not getting what they expect.) + +Due to all these problems with "context-shifting partials", for a long time the +core team has considered `{{partial}}` (in its current form) a legacy feature +and wanted to discourage its use in new apps. However, our deprecation policy +requires us to provide a transition path before deprecating any features, and +since all available alternatives today (see the "Alternatives" section) have +significant drawbacks, we were not able to deprecate this feature and communicate +our preference to the community. + +By introducing a non-context shifting form of `{{partial}}` that does not have +any of these drawbacks, we will be able to signal our intent to remove the +feature and transition the community to a better alternative immediately. + +# Detailed design + +Currently, the `{{partial}}` helper only take one positional arguments (the name +of the partial). We will use the presence of any named arguments as an opt-in for +the new semantics. + +Specifically: + +1. `{{partial "foo"}}` will invoke the `foo` partial template with the current + `this` as its context and make available any local variables that are in-scope + on the invocation side. + +2. `{{partial "foo" bar="baz"}}` will invoke the `foo` partial template with `this` + set to `{ bar: "baz" }` (a POJO consisting of only the named arguments passed to + the partial), and does not make any outer local variables available to the + template. + +Users who are familiar with how component invocations work today should find the +second form quite familar and easy to understand (which further highlights the +oddities of the first/current form). + +In addition, we will pair this change with a deprecation. When a partial template +is invoked in the first form **and** it attempts to access any variable on `this` +or on the outside scope (i.e. any `{{foo}}` that does not resolve into a helper), +we will issue a deprecation warning suggesting the user to switch to the new +invocation style. + +While the named arguments can be used as an opt-in for cases where external data +are needed, the `{{partial "foo"}}` inovocation is actually ambigious. In this +case, there are three possibilities: + +1. This is an existing "old-style" ("context-shifting") partial where the "foo" + template depends on access to the outside scope. + +2. This is an existing "old-style" ("context-shifting") partial where the "foo" + template does not depend on access to the outside scope (e.g. a site footer). + +3. This is a "new-style" ("non-context-shifting") partial where the "foo" + template _happens_ to not take any arguments (e.g. a site footer). + +In the first case, there will be a deprecation warning nudging the user to refactor +into the new-style invocation with named arguments. + +On the other hand, there are no ways to differentiate between the second and the +third case. *However, there are no observable differences between the two.* Notably, +there will be no deprecation (since the warning is triggered by actually accessing +the outer scope, not merely from invoking an old-style partial). Since there are no +external data-dependency, when Ember drops support for "context-shifting" partials, +the code will continue to work seamlessly even if the underlying implementation +strategy changes. + +Speaking of internal implementation strategy, the new-style partial as described in +this proposal can be implemented completely in "user-land" (as opposed to inside +Glimmer VM) with a combination of AST-transform and using Glimmer's component system: + +1. Introduce an AST-transform that turns any new-style partial invocation from + `{{partial name foo=bar ...}}` into `{{-internal-new-partial name foo=bar ...}}`. + +2. Implement `{{-internal-new-partial}}` as a new kind of light-weight component + using the component manager API. This light-weight component will simply capture + any named arguemnts and return it as the "self" reference and does not have any + template wrapper or lifecycle-hooks. + +The only exception is the deprecation message. Since the actual triage for `{{foo}}` +happens [deep inside the VM internals][resolve-maybe-local], we would probably have +to expose additional hooks (or other mechanisms) for Ember to issue to deprecation +warning. + +[resolve-maybe-local]: https://github.com/glimmerjs/glimmer-vm/blob/f78e91b47af9195d2ab063cb24e12ae5f1e5c9be/packages/%40glimmer/opcode-compiler/lib/syntax.ts#L272 + +# How We Teach This + +For new users, we can teach the new-style `{{partial}}` as if it is just a light-weight +(template-only) component that does not have/need any behavior assocaited with it. +Since they are actually implemented as components under-the-hood, it will be a completely +leak-free analogy. + +Because of that, it might even be a good training wheel for teaching components in Ember +to a completely new user: start with a big template, and when it grows *too* big, you can +extract it into a partial. Once you have done that, since you have already identified and +passed in any external data-dependency, you will be ready to "upgrade" to a component +whenever you need to add behavior (or adding CPs, etc) by (more-or-less) just adding a +JavaScript file and some minor changes to the invocation. + +It is also probably the case that for someone who is completely new to Ember, they would +have no reason to expect partials to work the way they do today – after all, it is quite +normal to have to explicitly pass in any data you need in every other context (component +invocation, function calls, etc). So, arguably, this is actually one *fewer* thing to +teach and might actually align better with everyone's metal model of how programming +works. + +For existing users who are already familiar with how components work, this should also +be quite easy to teach. Admittedly, the challenge (or potential confusion) arises from +having to "forget" how partials work today. This should be addressable with a clear +deprecation message. + +Also, as we have been promoting Ember's switch away from context-shifting helpers for +quite a while now, arguably it also just amounts to "forgetting a special exception to +the general rule", which should make this feels quite nice and coherent in the long run. + +Once you have completed the migration (following the deprecation warning), it will also +makes it much easier to refactor partials into components if/when there is a need for +them. + +# Drawbacks + +* "How is it different from a component?" + + The new-style component is, in fact, a component. The point of this proposal is to + (overtime) eliminate a special construct (partials) and unify them with an existing + one (components). + + It might have been nice if we could just remove it and replace it directly with Just + Components™, but we are not quite there yet (see the "Alternatives" section). + +* `{{partial "foo"}}` is ambigious + + As mentioned above, it is not possible to express "new-style partials but without any + arguments" due to the choice of the opt-in syntax. However, also as mentioned above, + there are no observable differences in semantics. It does mean that the "content-only + partials" will be a little slower than they need to be (since we must use the old + implementation due to the possible ambiguity), but this is unlikely to matter in + practice. + +* Creating extra churn when adding Glimmer components + + When "Glimmer components" land, the "new-style partial" functionailty will be rendered + obsolete as "template-only Glimmer components" will offer a superset of its features + and be just as easy to use. Presumably, when that happens, the partial feature will be + phased out and removed from core. Therefore, landing this feature now might introduce + extra churn. + + However, once our users have migrated to "new-style partials", it will be trivial to + upgrade them into Glimmer components with an automatic code mod (assuming [Module + Unification][module-unification]). The hard part in the transition is to figure out + the implicit data dependency from the "context-shifting partials", which needs to + happen no matter what. Therefore, in aggregrate there are very little wasted effort, + and this feature will serve as an important intermediate step in the transition. + + Furthermore, since "new-style partials" does not have any of the downsides of + "context-shifting partials", it becomes much less urgent to deprecate and remove + the feature, as the only motivation would be to reduce an redundant concept (as + opposed to discourage a hazardous programming model). Therefore, we can choose to + phase out the feature more slowly ("soft-deprecation"). + + Finally, since the feature is implementable in user-land (again, as opposed to + inside Glimmer VM), we can support this feature for the foreseeable future in a + supported addon separate from core when the time comes. (In fact, with the + [Custom Components API][custom-components] it will be possible to implement this + purely using Ember's public API alone.) + + [module-unification]: https://github.com/emberjs/rfcs/blob/master/text/0143-module-unification.md + [custom-components]: https://github.com/emberjs/rfcs/pull/213 + +# Alternatives + +The ultimate goal of this proposal is to depreacte "context-shifting partials" as +they exist today. The "new-style partials" propose exists merely to provide a +transition path to satisfy the requirements to deprecate the feature. Here are some +alternative transition paths: + +* Curly components + + If the goal is just to replace partials with components, we could simply advise + our users to "use components instead". In today's world, that would mean replacing + them with curly components (`Ember.Component`). + + The problem with curly components is that they will introduce a wrapper element by + default, which might break the page's layout/CSS. To work around that, they would + have to either add a JavaScript class for the compoment to write `tagName: ''` or + to supply that on the invocation side. + + The current consensus is that this is "too much of a lift" that makes an otherwise + very simple usecase feels "too heavy", especially when teaching partials as a simple + refactoring tool to new users. + +* Glimmer components + + Another option is to do nothing today and wait for Glimmer components to land, which + is the status quo. In my opinion, this has been the status quo for too long. It is + quite unfortunate that we were not able to actively discourage new partials from + being written when the majority of the core team and a large part of the community + are already aware of the harzards. In general, we should also decouple feature + dependencies as much as possible. + + As mentioned above, it should be possible to automatically migrate all "new-style + partials" to "template-only Glimmer components" with a codemod once they land (whereas + migrating "context-shifting partials" seems much more difficult). + + That being said, I definitely would like more eyes/brains on this claim to make sure + it is in fact doable, and where possible, we should (speculatively?) align the proposed + semantics with "template-only Glimmer components" to make our job easier in the future. + + For the codemod to work, there are two sides to the equation – the invocation sites + and the partial tempaltes. + + On the invocation side, as long as they all go through the `{{partial ...}}` keyword + (which would argue for disallowing invoking a new-style partial with the component + helper), they should be trivial to discover and rewrite. + + On the template side, as far as I am aware, the only deviation from "tempalte-only + Glimmer components" is instead of assigning and accessing the passed arguments via + `this`, they will likely be accessed through the `@foo` syntax (which is similar to + `attrs.foo` in curly components today). + + In theory, assuming the project has also adopted [Module Unification][module-unification], + it should be easy to identify all of the known helpers and partial templates in a + project as they will be located in their own collection. Once we have that information, + it should just be a matter of resolving each `{{foo}}` in all partial templates + statically and rewrite anything that is not a helper into `{{@foo}}`. + + However, for historical reasons, the internal partial template lookup rules are + currently quite "liberal", so identifying all partials templates of a project + might turn out to be challenging or even impossible in practice. (It is unclear + at this point how much of those will be carried over to the Module Unification + world.) + + Therefore, if possible, it is better to align the template-side semantics perfectly + so that the codemod would only need to worry about the invocations. One possibility + is to "fast-track" a proposal that simply enables the `@foo` syntax before landing + this proposal. This would allow us to drop the `this` POJO and allow for a seamless + upgrade to "template-only Glimmer components" (based on what we know abou them + today, which is subject to change of course). + + [module-unification]: https://github.com/emberjs/rfcs/blob/master/text/0143-module-unification.md + +* Disallow variables in partials + + Another option is to deprecate accessing *any* variables inside partials. This only + works for simple "content-only" partials (e.g. a site footer). However, there are + a lot of existing use cases for partials that does not fit that description. + +In addition, we could also... + +* Deprecate partials without providing an alternative + + This would violate our deprecation policy. + +* Keep the current `{{partial}}` indefinitely + + I already enumerated the problems with the features, but I would be curious to know + if anyone is interested in representing/advocating for this. + +# Unresolved questions + +* Should we drop the prefix `-` (in the template filename) requirement? +* Should we move partials into the components folder? +* What should happen if you do `{{partial-name ...}}`?