Skip to content
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

Deprecate {{with}} #445

Merged
merged 1 commit into from
Mar 16, 2019
Merged

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Feb 13, 2019

closes #316

Rendered

@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2019

Definitely in favor of migrating {{with -> {{if-let, thanks for picking this up @GavinJoyce!

@GavinJoyce GavinJoyce changed the title [WIP] deprecate {{with}}, introduce {{if-let}} Deprecate {{with}}, introduce {{if-let}} Feb 14, 2019
@GavinJoyce GavinJoyce force-pushed the gj/deprecate-with branch 2 times, most recently from 9be4de9 to 1f38a6d Compare February 14, 2019 10:15

This adds a litte API and documentation churn.

## Alternatives

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a very good reason for not doing it but just throwing this out there. Rather than increase the template API surface area could we enhance {{let}} with the {{else}} functionality?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patocallaghan see the motivation section from the {{let}} RFC:

While the conditional semantics of with are coherent with the other built-in helpers like each and if, users often find this unexpected. The fact that only the first positional parameter of with controls whether the block is rendered might also add to the confusion.

{{let}} was purposefully designed to always render its block.

@tcjr
Copy link

tcjr commented Feb 14, 2019

Deprecating {{with}} is a good idea. But {{if-let}} seems completely superfluous since you can get the desired behavior with {{let}} and {{if}}.

{{#let "Alex" as |value|}}
  {{#if value}}
    {{value}} is truthy
  {{else}}
    The first positional param was falsy
  {{/if}}
{{/let}}

If the legacy {{with}} didn’t exist, would we even consider adding {{if-let}} to core?

What about {{unless-let}}?

@adamseckel
Copy link

But {{if-let}} seems completely superfluous since you can get the desired behavior with {{let}} and {{if}}.

Another benefit of composing {{#let}} and nested {{#if}} is that you could handle multiple params and react to different positional params being false-y in different ways in the UI 🤔

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Feb 14, 2019

@tcjr Thanks, I'll add your suggestion to the list of alternatives. I crafted this RFC based on the comments in #316 (comment), it seemed to indicate the the core team were in favor of adding {{if-let}}.

I actually prefer your example of combining {{let}} and {{if}}, I think it reads better. Also, if we were to add {{if-let}}, I don't believe that it would be widely used. I can't think of a place in our own app where we would us it.

Another benefit of composing {{#let}} and nested {{#if}} is that you could handle multiple params and react to different positional params being false-y in different ways in the UI 🤔

@Hemlok 👍

This just came up again at a core team call, I think we are all 👍 to adding if-let (with today's {{with}} semantics) and deprecating {{with}} itself.

@rwjblue do you remember why there was support from the core team for adding {{if-let}}? Was a combination of let and if also considered?

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Feb 14, 2019

Thinking about it more, I strongly agree that we should just deprecate {{with}} and not introduce {{if-let}}. I'll update the RFC to reflect this now. If some good reasons emerge to introduce {{if-let}} and there is consensus, I can add it back to the RFC.

@kellyselden
Copy link
Member

@GavinJoyce Perhaps you could also alter the RFC to ensure public API exists so that and addon can implement if-let without hacks (might already be possible).

@GavinJoyce GavinJoyce changed the title Deprecate {{with}}, introduce {{if-let}} Deprecate {{with}} Feb 14, 2019
@GavinJoyce
Copy link
Member Author

@kellyselden here's a simple {{if-let}} component implementation:

https://ember-twiddle.com/9af662e85c5f06b6e4cb7f58ab08c954?openFiles=templates.application.hbs%2Ctemplates.components.if-let.hbs

screen shot 2019-02-14 at 13 27 26

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Feb 14, 2019

😜I got the naming wrong in the twiddle above, used {{if-let}} instead of {{let-if}}. I think that's another good reason not to introduce {{let-if}} in ember core.

EDIT: Oh, I actually got it right but thought that I got it wrong. The confusion point still stands

@rwjblue
Copy link
Member

rwjblue commented Feb 14, 2019

I think that @tcjr / @Hemlok's intuition here is good. Manually writing out the nested version of if-let is easy enough, is not much longer than the shorthand {{if-let version, and is probably more intuitive for folks that don't have experience with languages that themselves have a built in if-let (e.g. rust or some lisps).

tldr; 👍 on just making this a straight up deprecation RFC...

text/0000-template.md Outdated Show resolved Hide resolved

We'll mentiton the deprecation in an Ember point release blog post.

The deprecation message will link to good documentation which describe why `{{with}}` was deprecated and gives clear guidelines on how to migrate to using either `{{let}}` or `{{let}}` and `{{it}}` in combination.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include the deprecation guide prose in this section directly. The deprecation guide entry will be linked to directly from the deprecation itself, and should show before/after examples for the various styles of {{with usage.


## Unresolved questions

This [comment](https://github.com/emberjs/rfcs/issues/316#issuecomment-449489686) seems to indicate some support for `{{if-let}}` from the core team. Perhaps there are some compelling reasons for add it that are not currently included in this RFC?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like if-let, but I think that this is just as easy to understand and doesn't rely on the reader to grok extra concepts....

@dgeb
Copy link
Member

dgeb commented Feb 14, 2019

Good discussion. I also favor deprecating {{with}} without introducing a new replacement simultaneously.

@rwjblue rwjblue added T-templates T-framework RFCs that impact the ember.js library labels Feb 14, 2019
@rwjblue
Copy link
Member

rwjblue commented Feb 14, 2019

I'd also like to suggest creating a linting rule (to aid folks who are not yet on newer versions with the deprecation but can use {{let, to avoid {{with) and a codemod (leveraging codemod-cli + ember-template-recast should make this straight forward).

@rwjblue rwjblue self-assigned this Feb 14, 2019
@GavinJoyce
Copy link
Member Author

I'd also like to suggest creating a linting rule

and a codemod (leveraging codemod-cli + ember-template-recast should make this straight forward).

@rwjblue for the same reasons as why we need to provide three options in the deprecation guide, I'm not sure that it will be possible to fully automate the migration from {{with}} to {{let}}

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2019

@rwjblue for the same reasons as why we need to provide three options in the deprecation guide, I'm not sure that it will be possible to fully automate the migration from {{with}} to {{let}}

Well, there is a fully correct codemod that we could write. It would just look hideous in most cases when you didn't want the conditional behaviors.

For example:

{{#with (concat first ' ' last) as |name|}}
  <h2>Hello, {{name}}!</h2>
{{/with}}

Would have to be written as:

{{#let (concat first ' ' last) as |name|}}
  {{#if name}}
    <h2>Hello, {{name}}!</h2>
  {{/if}}
{{/let}}

Even though we mostly know that we don't need that conditional.


I think the codemod could actually have some pretty nice heuristics to be pretty close to what you would do by hand:

  • if the {{#with has an {{else, always emit the nested {{#if
  • if the argument to {{#with is a subexpression, assume that it is non-falsey (and therefore do not emit the nested {{#if inside the block)
  • otherwise use a "simple" {{#let without nested if

In addition, we could allow a flag to the codemod so that the person running it can opt in to "100% correctness, but super verbose" mode. The Ember.K codemod did essentially the same thing by requiring a flag to assume "chaining" or not (codemod docs here).

Note: I think that we would need to test these heuristics on a number of apps, auditing the results. Just to make sure they are right a significant majority of the time...

@GavinJoyce
Copy link
Member Author

I think the codemod could actually have some pretty nice heuristics to be pretty close to what you would do by hand

👍thanks, I've added a short note on adding a codemod to assist the migration

text/0000-template.md Outdated Show resolved Hide resolved
@chancancode
Copy link
Member

We discussed this today at the framework core team meeting and decided to move this RFC into Final Comment Period

@chancancode
Copy link
Member

We decided to accept this RFC at the framework core team meeting today. Thanks @GavinJoyce for working on this and everyone else for the discussion 🎉

@chancancode
Copy link
Member

@GavinJoyce do you mind renaming the file to match the RFC number?

@GavinJoyce
Copy link
Member Author

@chancancode updated, thanks

@chancancode chancancode merged commit ff83436 into emberjs:master Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate {{with}} helper
9 participants