Transition Away From Property Fallback Behavior #308
Conversation
| <h1>Hello !</h1> | ||
| ``` | ||
|
|
||
| This can be a refactoring hazord and can often lead to confusion for readers of the template. Upon encountering `{{greeting}}` in a component's template, the reader has to check all of these places: first you need to scan the surrounding lines for block params with that name; next you check in the helpers folder to see it there is a helper with that name (it could also be coming from an addon!); finally, you check the component's JavaScript class to look for a (computed) property. |
There was a problem hiding this comment.
Note, this problem makes it very hard to "tool" -- "surrounding block param" is purely local, so you can write a syntax colorer or completion tool that understands it. But other kinds of fallback are much less local and are therefore much harder to tool.
|
Thanks for writing this up @chadhietala! I am definitely in favor of reducing the mental overhead of scanning a template and "knowing" where things are coming from.... |
| - `{{@foo}}` is an argument passed to the component | ||
| - `{{this.foo}}` is a property on the component class | ||
| - `{{#with this.foo as |foo|}} {{foo}} {{/with}}` the `{{foo}}` is a local | ||
| - `{{foo}}` is a helper |
There was a problem hiding this comment.
I think we can merge the last two, so that {{foo}} is always "look up foo in the current scope", and if a variable is not in scope, it falls back to the file system "scope" to finish the search.
| - `{{foo}}` is a helper | ||
|
|
||
| # Drawbacks | ||
| The largest downside of this proposal is that it makes templates more verbose, causing developers to type a bit more. This will also create a decent amount of deprecation noise, although we feel like tools like [ember-cli-deprecation-workflow](https://github.com/mixonic/ember-cli-deprecation-workflow) can help mitigate this. |
There was a problem hiding this comment.
I think we would want to say that we must have a codemod before enabling the deprecation due to the noise it would cause. We should also think about whether we can scope the deprecation to the user's app; for example, we could consider making it a build-time deprecation, rather than a runtime deprecation.
| Angular relies heavily on the dependency injection e.g. `@Input` to enumerate the bindings that were passed to the component and relies heavily on TypeScript to hide or expose values to templating layer with `public` and `private` fields. Like Vue, Angular does not disambiguate. | ||
|
|
||
| ## Introduce Yet Another Sigil | ||
| We could introduce another sigil to remove ambiguity. This would address the concern about verbosity, however it is now another thing we would have to teach. |
There was a problem hiding this comment.
And in light of this. having a meaning in JS, and generally being perceived as relatively low-noise to JS developers, it seems worse to introduce another sigil.
|
It's worth adding "Do not change the default in Glimmer components" to the alternatives section. |
|
I'd also mention a codemod in the summary section, since without a codemod, this change will suck pretty hard. For context, I think we could use a "runtime aided" codemod here, where we boot the app, compile all the templates, and enumerate exactly where the Glimmer compiler resolved a |
| As an aside, the ambiguity that causes confusion for human readers is also a problem for the compiler. While it is not the main goal of this proposal, resolving this ambiguity also helps the rendering system. Currently, the "runtime" template compiler has to perform a helper lookup for every `{{greeting}}` in each template. It will be able to skip this resolution process and perform other optimizations (such as reusing the internal [reference](https://github.com/glimmerjs/glimmer-vm/blob/master/guides/04-references.md) | ||
| object and caches) with this addition. | ||
|
|
||
| Furthermore, by enforcing the `this` prefix tooling like the [Ember Language Server](https://github.com/emberwatch/ember-language-server) does not need to know about fallback resolution rules. This makes common features like ["Go To Definition"](https://code.visualstudio.com/docs/editor/editingevolved#_go-to-definition) much easier to implement since we have semantics that mean "property on class". |
There was a problem hiding this comment.
In other words, it makes resolving a name to one of these purely local:
- local variable
- property on the component
- external helper or component
|
I think this will represent an significant reduction in ambiguity in reading templates in Ember that will be especially helpful for developers new to the framework. Our main app is now 5 years old, currently on Ember 2.18 and I can see us adopting this very shortly after it lands. |
| The follow is a breakdown of the different forms and what they mean. | ||
|
|
||
| - `{{@foo}}` is an argument passed to the component | ||
| - `{{this.foo}}` is a property on the component class |
There was a problem hiding this comment.
why is it this.foo, but not this.@foo?
There was a problem hiding this comment.
My understanding is that this. makes reference to be a property in the component instance, and @foo-like arguments do not require a component to even exist, like it will happen in components without a javascript file, that become something similar to functional partials.
|
Any reduced resolving complexity makes future tree-shaking of templates easier. Perhaps that can be added to motivations, or it's too handwavy and we can leave it out. Just thinking out loud. |
|
I am for this RFC. It brings to mind the Not Explicit blog post from a member of the Rust community. I like how the RFC allows us to know more about templates by reading solely their source. I feel like the disambiguation and alignment with JavaScript syntax described by this RFC will also help prepare the transition to one-way bound Glimmer templates by widening the gap between properties of the component's JavaScript class, and arguments passed in. |
|
I like this, it will make reading templates much easier at a glance. Does this RFC address the same problem (helper name vs. property lookup) in controllers? |
|
I like this a lot. |
|
I'm worried the verbosity issue and deprecation spew will lead to a lot of complaints. |
|
👍 My app just broke when I installed ember-math-helpers. A template Edit: I found another case which wasn't caught by my test suite and ended up uninstalling the addon due to this issue |
|
@cibernox I believe the plan will be to launch the app with Puppeteer and collect the information. Once that is done, use that information to inform the codemod. This should cover a very large portion of the cases. |
|
This solves a non-existent problem (I haven't encountered it once, and I don't know of anyone that has, really - just don't name your properties after those 20, let's say 50 helpers tops you usually use - it's not that hard), and adds a huge amount of completely arbitrary verbosity to templates. |
|
I would greatly appreciate this— I've bumped into issues around this several times when making addons. For ember-page-title I used a handlebars plugin to disambiguate calls between ✨ Thanks so much for putting this together @chadhietala ! ❤️ |
There was a problem hiding this comment.
Let me start by saying that if I was starting a new app now, I would think this was a good change.
I'm not tho 😄. This will really hurt us a lot unless you can guarantee that the code-mod will work in every instance. Even then, we'd likely have to audit most of the components in our app to make sure everything still works after migrating. Tests would catch a lot but not everything. The benefits however seem relatively minor compared to the effort involved.
The fact is, when you add a deprecation you're creating an expectation that people will migrate. Sooner or later people with large/old apps would have to deal with this or get left behind.
For this reason, I propose the following:
- The docs use the
{{this.x}}syntax so new users will do it 'properly' from the start if they follow "official" documentation. - Issue a deprecation only when a lookup is ambiguous explaining how to disambiguate.
- Still provide the code-mod so people can migrate easily if they want.
I really hope this doesn't come across too negatively. I generally like the intent of the change, but given the relatively minor benefits for existing applications and the massive amount of bindings in even a small app, I think it doesn't make sense to deprecate this type of use all in one go.
| - `{{#with this.foo as |foo|}} {{foo}} {{/with}}` the `{{foo}}` is a local | ||
| - `{{foo}}` is a helper | ||
|
|
||
| # Drawbacks |
There was a problem hiding this comment.
A drawback I think is missing: copying old examples will now lead to deprecation warnings. This will probably confuse new-comers who are learning Ember. There are a lot of blogposts, stack overflow answers, video courses, etc. that probably use the old syntax. I think it's very confusing for beginners to see deprecation warnings when they just started following some basic examples.
There was a problem hiding this comment.
I couldn't agree more @joukevandermaas. That was one of my biggest hang-ups when I got started (around 1.13, still a noob).
|
Since I originally posted this RFC I believe folks have raised some legitimate concerns about how to transition existing applications and the ecosystem of addons to require the Over the past month I have been working on a tool called Dyfactor that allows you to leverages runtime information to perform static analysis. With this system we are able to write 2-phased plugins. The first phase is to instrument an application and launch it with Puppeteer. The second phase is the actual codemod phase. Unlike traditional codemods, these codemods are given access to the telemetry data collected from runtime. To prove this out I wrote a plugin to transition the templates used on the homepage of emberobsever to the While this tool is only as good as the amount of permutations you give it, I feel that it drastically closes the gap on the migration concerns. |
This updates the transition section to explain the slow roll out all the way to it's removal in 4.4.0.
chadhietala
changed the title
|
After discussion with the core team this afternoon, we are 👍 on moving this RFC into the final comment period. We understand this is a large change and thus we have outlined what the rollout strategy will be. |
|
I think people will get used to the |
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| # Summary | |||
|
|
|||
| Deprecate the fallback behavior of resolving `{{foo}}` by requiring the usage of `{{this.foo}}` as syntax to refer to properties of the templates' backing component. This would be the default behavior in Glimmer Components. | |||
| Beging the transition to deprecate the fallback behavior of resolving `{{foo}}` by requiring the usage of `{{this.foo}}` as syntax to refer to properties of the templates' backing component. This would be the default behavior in Glimmer Components. | |||
There was a problem hiding this comment.
"Beging" -> "Beginning" ?
|
I have been using this syntax in one new project and I have to say that I love it. It makes the templates feel more natural. |
|
@chadhietala looking at the video with Dyfactor, would it be possible to use this and run the test suite for the app? While this may not hit ever permutation of a template, for tested apps it could be a way to cover most of the application surface. |
|
@rtablada - Yep, thats definitely something that should work. |
|
Why removing the fallback at |
There was a problem hiding this comment.
@luxferresum - I commented inline with a few tweaks to hopefully make it clearer, but the reason that the fallback wouldn't be removed from the codebase until 4.5 ("after 4.3" was a typo that I commented on), is that we typically attempt to extend support for "legacy APIs" through the first LTS of a new major version. You can see that in the 2.0 and 3.0 release timelines, both had "ember legacy" addons that you could use to opt-in to extend the older APIs for a small bit longer...
| - Rev major to 4.0.0 | ||
|
|
||
| **Phase 7:** | ||
| - Remove fallback functionality post 4.3.0 LTS |
There was a problem hiding this comment.
The LTS would be 4.4.0, and the fallback could be removed in 4.5.0.
| - Introduce deprecation for **any** fallbacks | ||
|
|
||
| **Phase 6:** | ||
| - Rev major to 4.0.0 |
There was a problem hiding this comment.
Should mention here that the fallback would now assert unless the legacy addon is present.
There was a problem hiding this comment.
THX, the hint for the legacy addon here is what I was missing.
|
Can this be merged or does FCP need to be restarted? |
|
@knownasilya - We need to tweet out RE: the FCP (from the emberjs account), and I keep forgetting to get it done. AFAIK, there isn't anything blocking other than that procedure though... |
|
We reviewed this at todays core team meeting, and are still in favor of moving forward. This RFC process was super helpful in coming up with a rollout strategy that works for everyone, thank you! |
As part of [emberjs/rfcs#308](https://emberjs.github.io/rfcs/0308-deprecate-property-lookup-fallback.html#transition-path)'s phase 2. The main guides/docs have been updated (will be deployed with 3.7 docs) in ember-learn/guides-source#349.
|
there is no ember issue referenced for this RFC. Has the implementation work already started? |
no issue We were in a part-way state where some touched files had been (sometimes partially) migrated to explicit `this`. The codemod that was available has now fixed the formatting issues it had so it was a good time to run it. https://github.com/ember-codemods/ember-no-implicit-this-codemod - part of the migration path for emberjs/rfcs#308 - starts to make template resolution rules more explicit - `<MyComponent />` - always a component - `{{my-component}}` - component or helper (components _must_ have a `-`. This style of component will go away once fully migrated to angle bracket components) - `{{value}}` - a helper or local template variable - `{{this.value}}` - reference to a property on the backing context (either a controller or a component JS file)
Lands the deprecation proposed in [emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md): The [no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md) template linting rule has been enabled for Ember applications by default since around Ember 3.16 (included in the `octane` preset). This PR will take that a step further and issue a deprecation for templates that are still using `{{foo}}` to mean `{{this.foo}}`. Note: It does provide for an easy way to temporarily disable the deprecation messages (e.g. to reduce console output to hone in on other issues). This is intended for sporadic usage while debugging **not** as a way to mitigate the deprecation (the fallback behavior will be removed in 4.0.0).
Lands the deprecation proposed in [emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md): The [no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md) template linting rule has been enabled for Ember applications by default since around Ember 3.16 (included in the `octane` preset). This PR will take that a step further and issue a deprecation for templates that are still using `{{foo}}` to mean `{{this.foo}}`. Note: It does provide for an easy way to temporarily disable the deprecation messages (e.g. to reduce console output to hone in on other issues). This is intended for sporadic usage while debugging **not** as a way to mitigate the deprecation (the fallback behavior will be removed in 4.0.0).
Lands the deprecation proposed in [emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md): The [no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md) template linting rule has been enabled for Ember applications by default since around Ember 3.16 (included in the `octane` preset). This PR will take that a step further and issue a deprecation for templates that are still using `{{foo}}` to mean `{{this.foo}}`. Note: It does provide for an easy way to temporarily disable the deprecation messages (e.g. to reduce console output to hone in on other issues). This is intended for sporadic usage while debugging **not** as a way to mitigate the deprecation (the fallback behavior will be removed in 4.0.0).
Lands the deprecation proposed in [emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md): The [no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md) template linting rule has been enabled for Ember applications by default since around Ember 3.16 (included in the `octane` preset). This PR will take that a step further and issue a deprecation for templates that are still using `{{foo}}` to mean `{{this.foo}}`. Note: It does provide for an easy way to temporarily disable the deprecation messages (e.g. to reduce console output to hone in on other issues). This is intended for sporadic usage while debugging **not** as a way to mitigate the deprecation (the fallback behavior will be removed in 4.0.0).
Lands the deprecation proposed in [emberjs/rfcs#308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md): The [no-implicit-this](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-implicit-this.md) template linting rule has been enabled for Ember applications by default since around Ember 3.16 (included in the `octane` preset). This PR will take that a step further and issue a deprecation for templates that are still using `{{foo}}` to mean `{{this.foo}}`. Note: It does provide for an easy way to temporarily disable the deprecation messages (e.g. to reduce console output to hone in on other issues). This is intended for sporadic usage while debugging **not** as a way to mitigate the deprecation (the fallback behavior will be removed in 4.0.0).
Rendered