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

{{else}} block for {{component}} helper #12250

Closed
wants to merge 1 commit into from

Conversation

xcambar
Copy link
Contributor

@xcambar xcambar commented Aug 30, 2015

Answering my own needs (see http://discuss.emberjs.com/t/failsafe-component-helper/8705),
I added the {{else}} block for the {{component}} helper; this block is rendered when Ember is not able to lookup the component.

This solves a different use case that when null or undefined is given to the helper as a component name. It allows much simpler CPs to generate component names (without component lookups) yet with a failsafe display.
The updated docs will certainly explain it all much better :)

I will also take the opportunity to discuss about the values null or undefined.
I didn't change the existing behaviour but I'm inclined to think it would fit better in the {{else}} block (giving responsibility to the developers) than in the internals of the helper. What do you think?

```

Note: When the name of the component is `null` or `undefined`, nothing is rendered.
Copy link
Member

Choose a reason for hiding this comment

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

This works without the 'Note:' prefix, can you remove the prefix?

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2015

This looks great, I definitely think this is a nice addition.

Since this adds new API, can you please add a feature flag (perhaps named 'ember-htmlbars-component-else') for the new behavior?

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2015

Can you also add a test showing that starting with the else block rendered can leave the else block state when a component can be found (there is already a test for the other way around)?


// If the value passed to the {{component}} helper is undefined or null,
// don't create a new ComponentNode.
if (componentPath === undefined || componentPath === null) {
return;
}

env.hooks.component(morph, env, scope, componentPath, params, hash, { default: template, inverse }, visitor);
const result = lookupComponent(env.container, componentPath);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can avoid this temp variable via destructuring:

const {
  component,
  layout
} = lookupComponent(..........);

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2015

I did think of one notable difference here: the helper now does a container lookup for every render/rerender. I am not sure that is an issue at all, but I did want to point it out.

We might be able to mitigate by moving some of the work to setupState instead of render...

@mmun
Copy link
Member

mmun commented Aug 30, 2015

The else block should be passed to the component for use with {{yield to="inverse"}}. Looking up an invalid component is dubious. It's equivalent to

{{#foo-bar}}{{else}}If the foo-bar component doesn't exist show this instead!{{/foo-bar}}

@mmun
Copy link
Member

mmun commented Aug 30, 2015

You could instead implement the behavior of this PR with a helper.

{{#if (is-a-real-component type)}}
  {{component type ...}}
{{else}}
  Invalid component type: {{type}}
{{/if}}

@xcambar
Copy link
Contributor Author

xcambar commented Aug 30, 2015

@rwjblue I applied your remarks, except for setupState, as it didn't seem crucial as is. Let me know otherwise.

@mmun You're most certainly right. But the same way each has its else, I thought component could use it as well.

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2015

@mmun - There is no public API for determining if a component is available for a given string (container is private, component-lookup:main is private, etc).

@mmun
Copy link
Member

mmun commented Aug 30, 2015

@rwjblue So let's fix that instead.

@rwjblue
Copy link
Member

rwjblue commented Aug 30, 2015

@mmun - Meh. It seems reasonable for {{component}} to support an inverse. It seems much more in line with our other helpers (things like if, with, each, etc). What is your reasoning for opposing?

@mmun
Copy link
Member

mmun commented Aug 30, 2015

Simple. {{component "foo-bar"}}...{{else}}...{{/component}} should act exactly the same as {{#foo-bar}}...{{else}}...{{/foo-bar}}. Have fun teaching otherwise. The if, with, each are different. They are control flow. component is about delegation.

@rwjblue
Copy link
Member

rwjblue commented Aug 31, 2015

@mmun - OK, that is a wonderful reason!! Why didn't you say that? 😜

@xcambar - Merging this, would actually break the ability of the component itself to render an inverse block (and be a SemVer violation since this does work properly today (see JSBin demo)).

We need tests to confirm that {{component}} helper allows usage of {{yield to="inverse"}}, to make this clearer for future travelers....

@mmun
Copy link
Member

mmun commented Aug 31, 2015

@rwjblue am bad at words n stuff

@xcambar
Copy link
Contributor Author

xcambar commented Aug 31, 2015

👍 I didn't even know about {{#foo-bar}}...{{else}}...{{/foo-bar}}!

Before it's too late to ask, isn't it kind of semantically weird, this else?
I'm well aware that it may just be me, but the JSBin gave me more of a "Huh what?" moment than a "Aha" moment...

@xcambar
Copy link
Contributor Author

xcambar commented Aug 31, 2015

Rephrasing: "Can you help me understand the use case for {{#foo-bar}}...{{else}}...{{/foo-bar}}, please?"

@rwjblue
Copy link
Member

rwjblue commented Aug 31, 2015

@xcambar It allows things like {{liquid-if}} or {{has-permission}} to be components that have an inverse template.

@mmun
Copy link
Member

mmun commented Aug 31, 2015

@xcambar In my opinion, you're totally right that it's a bit weird. It happened organically to solve issues at the time (like the ones @rwjblue listed). A more general solution is being considered (code name: "named blocks", but it's still in the brainstorming stage). The tl;dr is that {{yield}} will invoke the "main" block and {{yield to="inverse"}} will invoke the else/inverse block. Named blocks would possibly add {{yield to="some-other-name"}}.

@xcambar
Copy link
Contributor Author

xcambar commented Aug 31, 2015

Thanks. I have a much better understanding now.

Fel free to close the PR. 👍 to @mmun for having more APIs public to allow the originally proposed behaviour, then.

@xcambar
Copy link
Contributor Author

xcambar commented Aug 31, 2015

FYI, I've just published ember-cli-if-component that has all the required features (hopefully 😉 ).

I'll be very happy to have your feedback on it.

@rwjblue
Copy link
Member

rwjblue commented Sep 1, 2015

@xcambar / @mmun - Thank you both for being awesome here.

@rwjblue rwjblue closed this Sep 1, 2015
@xcambar xcambar deleted the component_helper_with_else branch September 1, 2015 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants