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

Optional closure actions #90

Closed
rlivsey opened this issue Sep 11, 2015 · 4 comments
Closed

Optional closure actions #90

rlivsey opened this issue Sep 11, 2015 · 4 comments

Comments

@rlivsey
Copy link

rlivsey commented Sep 11, 2015

Short version

Closure actions break if you give them an undefined value, making it impossible to have optional actions:

{{x-foo on-bar=(action attrs.not-passed baz)}}

Long Version

Consider a hypothetical image component which sends an action when it's loaded:

{{x-image image=image on-load=(action "imageLoaded")}}

Now consider an image-list component which displays a list of images:

{{#each attrs.images as |image|}}
  {{x-image image=image on-load=(action "imageLoaded" image)}}
{{/each}}

So far so good, now this component wants to just pass the loaded event up to its parent component.
Nice and easy to do with closure actions:

{{#each attrs.images as |image|}}
  {{x-image image=image on-load=(action attrs.on-image-loaded image)}}
{{/each}}

This is called as so:

{{x-image-list images=images on-image-loaded=(action "imageLoaded")}}

But we don't want to require having on-image-loaded being passed in, this now breaks:

{{x-image-list images=images}}

I know, we'll make the action optional:

{{#each attrs.images as |image|}}
  {{x-image image=image on-load=(if attrs.on-image-loaded (action attrs.on-image-loaded image))}}
{{/each}}

No dice, (if) isn't lazily evaluated, so the (action) helper gets called with undefined regardless.

Now we're stuck, we can't use closure actions anymore and have to go back to using (action "imageLoaded") and sendAction which is happy if the action isn't wired up.

Possible Solutions

In order of personal preference:

1. Make (if) lazily evaluate

Currently the (if) helper executes both branches before testing (as helpers receive values and not streams), regardless of whether the predicate is interpreted as true or false.

If this instead lazily evaluated then it would be safe to use the action keyword inside an if as the action keyword would only be called if valid:

{{x-image on-load=(if attrs.on-image-loaded (action attrs.on-image-loaded image))}}

2. Add optional=true flag to action keyword

{{x-image on-load=(action attrs.on-image-loaded image optional=true)}}

This would disable the checks in the action keyword.

I personally prefer this to 3. because it doesn't require an entirely new keyword but still makes it clear what's going on.

3. Add (optional-action) helper/keyword

{{x-image on-load=(optional-action attrs.on-image-loaded image)}}

This simply guards against undefined actions before passing along to the action keyword, but is an entirely new helper/keyword just for a guard clause

@rlivsey
Copy link
Author

rlivsey commented Sep 11, 2015

I guess a 4th possibility would simply to allow undefined actions being passed into the (action) keyword, but that's probably too much of a potential foot-gun.

@rwjblue
Copy link
Member

rwjblue commented Sep 11, 2015

I think #3 is the right solution:

  • It is much more declarative (you can clearly see in the template that this is an optional action)
  • It allows us to still provide very nice error/warning messages for the default case (where you definitely expect the action to exist)
  • It is possible to do today without changes 😸
App.OptionalActionHelper = Ember.Helper.helper(function([action]) {
  return action || function() { };
});

Demo: http://rwjblue.jsbin.com/kuhesa/edit?html,js,output

@rlivsey
Copy link
Author

rlivsey commented Sep 11, 2015

@rwjblue I hadn't thought of just returning an empty function, great idea!

My idea for #3 was that it would call through to the action keyword, so in your example:

{{x-foo fire-away=(action (optional-action 'foo') baz)}}

would be

{{x-foo fire-away=(optional-action 'foo' baz)}}

but yours is much nicer/cleaner as just a separate helper!

Wonder if this should be built-in, or at least documented with closure actions as it seems like something which would be fairly common (unless I'm the only one doing this!).

@rlivsey
Copy link
Author

rlivsey commented Sep 11, 2015

Closing this for now, the helper solution works perfectly. Maybe in future a similar helper / syntactic sugar could be built-in but it's trivial to add to an app in the meantime.

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

No branches or pull requests

2 participants