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

fn & on #912

Merged
merged 9 commits into from Aug 8, 2019

Conversation

@NullVoxPopuli
Copy link
Contributor

commented Jul 4, 2019

@mixonic

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@NullVoxPopuli @bound is merely a proposal afaik, I'm not even sure if it is even a concrete proposal to TC39. I think the guides should stay focused on how to get thing done, and avoid contributing to FUD about what the future will bring.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Gotchya.

yeah, after reading the propoal for decorators closer, it looks like bound would be something ember may provide, given this implementation example from the decorators proposal:

export decorator @bound {
  @initialize((instance, name) => instance[name] = instance[name].bind(instance))
}

These are the only decorators being proposed as part of the proposal:
image

@@ -297,7 +297,7 @@ import { action } from '@ember/object';
export default class SendMessage extends Component {
@action
sendMessage(messageType) {
async sendMessage(messageType) {

This comment has been minimized.

Copy link
@mixonic

mixonic Jul 5, 2019

Contributor

I know some work supporting async/await landed into backburner and Ember in 3.x, but I'm not sure if FW core is encouraging this in app code yet. I've queried a few people for clarification.

Regardless, I'm not sure why this action is async... Simply to provide an example that it is possible?

This comment has been minimized.

Copy link
@NullVoxPopuli

NullVoxPopuli Jul 5, 2019

Author Contributor

I've been using async/await everywhere in emberclear, and haven't ran into issues. 🤷‍♂

I think the the name implies that there is going to be some interaction with a backend, so I would expect it to be async :)

Note that while Ember currently permits you to add an action to any DOM element, not all DOM elements are eligible to receive focus, according to HTML standards.
Note that while Ember currently permits you to add an action to any DOM
element, not all DOM elements are eligible to receive focus, according to
HTML standards.

This comment has been minimized.

Copy link
@mixonic

mixonic Jul 5, 2019

Contributor

This Note is pretty confusing. As a reader I'm not sure what this comment (something something focus) has to do with actions on non-clickable elements.

This is likely trying to nudge the user to think about accessibility? Can it be structured so that a first time using coming to this section can get the information they expected before that consideration distracts them?

This comment has been minimized.

Copy link
@NullVoxPopuli

NullVoxPopuli Jul 5, 2019

Author Contributor

This Note is pretty confusing.

yeah, I struggled with it a bit, too.

This is likely trying to nudge the user to think about accessibility?

I think so. I've primarily only used Chrome and FireFox, so I haven't run in to any issues with browsers not letting things be clickable. I wonder if this text is counting a screen reader or other a11y tool as a browser.

This comment has been minimized.

Copy link
@NullVoxPopuli

NullVoxPopuli Aug 2, 2019

Author Contributor

Not sure what to do about these notes -- kinda out of scope for this Pr, but if someone else wants to tackle it / open an issue for it, be my guest!

@CvX

This comment has been minimized.

Copy link

commented Jul 22, 2019

Could/should the docs provide a migration guidance for the value="target.value" pattern?

For example, given this code:

<input onclick={{action "update" value="target.checked"}} type="checkbox">
<input oninput={{action "update" value="target.value"}}>
actions: {
  update(value) {
    console.log(value);
  }
}

Is this the recommended replacement?

<input {{on "click" this.update}} type="checkbox">
<input {{on "input" this.update}}>
@action
update({ target: { checked, value } }) {
  console.log(checked || value);
}

And is there a path for (mut) and value combo? 🙂 For example:

<input oninput={{action (mut this.query) value="target.value"}}>

You'd have to create an action in the component that updates this.query, right?

AFAICT, the {{on}} RFC didn't touch on that? @pzuraq

@pzuraq

This comment has been minimized.

Copy link

commented Jul 22, 2019

@CvX I believe the first few examples should be the preferred form. Currently, we don't have a path to a direct replacement for mut/value, I agree this is a bit regrettable but we did explore this significantly during the design portion of the on and fn RFCs. The issue is that the current API is very counterintuitive:

  • mut does nothing directly on its own, it returns a function that sets a value. You still have to apply that function. This means that you have to know how the details of the surrounding environment work, which takes us to action and value
  • value is a really weird API, basically allowing you to reach into an event and grab a nested path to pass to the next thing. You need to know that the path will be followed on the event object itself, which is something that people trip up over frequently. It also does nothing valuable by itself - why wouldn't you just want the event in an event handler? It only makes sense in the context of the mut helper.

Basically, these APIs are convenient in that they are terse, but they are not easy to teach, and do not have much utility unless they are used together. We realized through the design process that what we really want here is some form of closures:

<input {{on "input" (|event| (set this.query event.target.value))}} />

Something along these lines. However, this is a huge lift, and adds a large amount of complexity to both Glimmer from an implementation perspective and templates from a language design persective, so I'm not sure if this is a direction we want to go yet.

I think in the meantime, it would be best to make a custom helper that handles common use cases like this, where it would be inconvenient to create an action handler. call it pickAndSet:

<input {{on "input" (pickAndSet this "query" "target.value")}} />

This would pick the value off of whatever is passed to it and set it. Then at least the two actions are related to each other lexically, and can tell the users that they are related just by reading it. You could use a template transform to mimic the mut helper if that would be clearer:

<input {{on "input" (pickAndSet this.query "target.value")}} />

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review Aug 2, 2019

@NullVoxPopuli NullVoxPopuli force-pushed the NullVoxPopuli:fn-n-on branch from 4146bb8 to 78c3e06 Aug 2, 2019

@NullVoxPopuli NullVoxPopuli force-pushed the NullVoxPopuli:fn-n-on branch from 78c3e06 to 4db5166 Aug 2, 2019

@@ -21,22 +21,30 @@ export default class Post extends Component {
}
```

You can then add this action to an element using the
[`{{action}}`](https://api.emberjs.com/ember/release/classes/Ember.Templates.helpers/methods/action?anchor=action)
`@action` binds the `this` of `toggleBody()` to the instance of the class, allowing access the component's other properties when invoked. Without the decorator, `this.isShowingBody` in `toggleBody()` is undefined. For more information on `this` [Understanding Javascript Function Invocation and this](https://yehudakatz.com/2011/08/11/understanding-javascript-function-invocation-and-this/)

This comment has been minimized.

Copy link
@muziejus

muziejus Aug 2, 2019

Thanks @NullVoxPopuli and @locks for talking this through with me. I think this hits the key terms (binds, class) while also, with the second sentence, explaining in a more quotidian way why @action is needed!

NullVoxPopuli added some commits Aug 2, 2019

around as values, and allow them to be invoked in different locations.

This comment has been minimized.

Copy link
@muziejus

muziejus Aug 7, 2019

that comma before "and" is not needed and causes confusion.

@@ -142,4 +142,5 @@ In the following guides we'll talk about:
which Ember fully supports

This comment has been minimized.

Copy link
@muziejus

muziejus Aug 7, 2019

While at it, this should have a period at the end to follow the style of the rest of this list.

@jenweber jenweber merged commit 8ed95f9 into ember-learn:octane Aug 8, 2019

5 of 7 checks passed

Header rules - ember-guides No header rules processed
Details
Pages changed - ember-guides 3660 new files uploaded
Details
Mixed content - ember-guides No mixed content detected
Details
Redirect rules - ember-guides 2 redirect rules processed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/ember-guides/deploy-preview Deploy preview ready!
Details
percy/guides-app Visual review approved by Jen Weber
Details
@jenweber

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Thank you @NullVoxPopuli and reviewers! If anyone wants to extend upon this work or make additional fixes, I'm happy to help get more PRs merged in!

@amyrlam amyrlam referenced this pull request Aug 9, 2019
7 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.