ui: {{phrase-editor}} amends

[johncowen]

This PR adds minimal amends/improvements to {{phrase-editor}}

1. Re-focus the input element on phrase removal (so when you click the cross button, it refocuses the input field ready to type)
2. Move all 'actions' to actions:{}
3. Move to a 'form looking/standard' value rather than items
4. Move placeholder functionality into the component
5. Force DDAU instead of two way binding with slice and onchange
6. Begin to deprecate the searchable interface

Also adds a single CSS value to fix a regression (the text within the buttons shouldn't be displayed)

[DingoEatingFuzz]

Looks good, but I think the changes warrant an integration test or two.

[DingoEatingFuzz]

What does this TODO mean?

[johncowen]

It's a reference to {{ref}}

https://octane-guides-preview.emberjs.com/release/components/glimmer-components-dom/#toc_communicating-between-elements-in-a-component

I added a little more detail to the comment in another commit

[DingoEatingFuzz]

ooOOOoo, so Reacty. I'm looking forward to that.

[DingoEatingFuzz]

This should either be this.send('remove', get(this, 'value').length - 1) or remove should be pulled out of actions.

[johncowen]

Reading up on send it seems to be used for bubbling messages up through multiple components and/or controllers and routes.

That's not what I want to do here.

I want to call a function on the same component, that for 'reasons' live on a different object within the same component.

As they live on a different object, and ember does its magical ✨ rebind when you use them via {{action}},this within the function doesn't mean what one thinks it means.

bind is the standard way of setting this to whatever you want it to be. So I use that to set this to what it is when you use {{action}}. No bubbling, no removing the action and not realizing that I have a similarly named action on the controller and things still 'working' but in the wrong way without me realizing (which I believe could be a footgun with send?)

Why not pull it out of actions? As of ember 2.18 actions are the way of saying 'hey template 👋 , these are my public API methods, I have 3 dragons hiding in here so please don't use anything else!' 😄 So right now I generally favour them (as of 2.18), even though (I believe?) the actions object is being removed.

sendAction has also been deprecated and I have a feeling send will probably go the same way soon considering embers tendency lately to favour a more standard javascript approach, and the speed at which ember is trying to deprecate things lately, I generally feel its safer to use standard javascript where possible (I think the majority of my app is deprecated or soon to be deprecated at this point 😅 ).

So they are my reasons for using standard bind, but if you still feel strongly that it needs changing then I'm happy to do so.

I suppose the question is, does send give me anything extra here that I need apart from it just being 'the ember 2.18 way'? In the same way that actions is 'the ember 2.18 way', but it also gives me a pseudo public interface as an extra benefit.

Anyway I'm going to see if I can add some testing, so I'll hold off on a merge here, lets catch up later.

P.S. Yes I am watching GoT 😄

[DingoEatingFuzz]

It's confusing since send in controllers/routes is different than send in components, but send in components doesn't bubble out. You need sendAction for that. All that send does in a component is make it so you don't have to do the bind dance.

Beyond looking nicer and not creating a local bound function, by using the public API, if this changes in the future (e.g., actions deprecated) you can expect a codemod to work.

Here's a minimal example from Nomad: https://github.com/hashicorp/nomad/blob/master/ui/app/components/two-step-button.js#L45

[johncowen]

Hey,

Thanks for the extra opinion here. I read up a little more on this:

Component::send
https://api.emberjs.com/ember/3.10/classes/Component/methods/send?anchor=send

Controller::send
https://api.emberjs.com/ember/3.10/classes/Controller/methods/send?anchor=send

Both of the above documentation pages use exactly the same documentation, and both link to exactly the same source code, they both bubble out:

https://github.com/emberjs/ember.js/tree/v3.10.2/packages/@ember/-internals/runtime/lib/mixins/action_handler.js#L167

So as far as I understand they both do exactly the same thing? The thing they add over bind is mainly the potential bubbling.

https://github.com/emberjs/ember.js/blob/41f1657f68735f214efeaa96eac96e8e093b4981/packages/%40ember/-internals/runtime/lib/mixins/action_handler.js#L199-L220

send(actionName, ...args) {
    assert(
      `Attempted to call .send() with the action '${actionName}' on the destroyed object '${this}'.`,
      !this.isDestroying && !this.isDestroyed
    );
    if (this.actions && this.actions[actionName]) {
      let shouldBubble = this.actions[actionName].apply(this, args) === true;
      if (!shouldBubble) {
        return;
      }
    }

    let target = get(this, 'target');
    if (target) {
      assert(
        `The \`target\` for ${this} (${target}) does not have a \`send\` method`,
        typeof target.send === 'function'
      );
      target.send(...arguments);
    }
  }

So, looking at the above code what does send give me extra? i.e. why should I use it?

We can see that send basically binds this to the actions object:

// create a local bound function
this.actions[actionName].apply(this, args)
//or actionName = 'remove';
this.actions[actionName].bind(this)(...args)

There is a defensive assertion to check for the object having been destroyed, which I believe gets compiled out? Plus this check is only useful if you have asynchronicity involved, which we don't here (and I'm avoiding in all my components or using events to organize). And then of course it adds the bubbling functionality, which I don't need here.

I've actually just realized the sideeffect-y target property on the object also, so if I was to use send I'd have to remember not to use/be careful using that target property myself in the future (it essentially should be private). I know of at least one place where I've used this already without knowing that its used for other things:

consul/ui-v2/app/components/event-source.js

Line 35 in 553d3cc

set(this, 'target', $previous);

Before reading the above I was all for switching, but it's confusing enough to be not immediately clear what it does in either components/routes/controllers. I don't mind writing bind, it looks more javascript-y to me and potentially to other ember and non-ember folks, most javascript devs will know what bind does but possibly not send. send does exactly the same, but adds things I don't want/need, and could potentially cause confusing sideeffect-y problems that wouldn't immediately make sense unless I'd done this little investigation. This final thing is the thing that worries me the most as I could have quite easily walked into the target problem by accident by using send over bind. It's gone from a thing that 'probably doesn't matter either way', to 'potentially could cause issues'.

Lastly, thanks for the discussion here, I really do appreciate it and it leads to this super interesting insights into ember itself. Lots of TIL 🙌

Thanks,

John

[DingoEatingFuzz]

Yeah, the target detail is one of a handful of sneaky keys you aren't allowed to use (some blow up more spectacularly than others). I really wish all user-land props were in their own object, but hindsight 20/20 I guess.

Just for further clarification/internals details...

Bubbling will only happen if target is set and the action being sent returns true. Target, although possible to be set by a user, is generally set by the framework.

Target is never set by the framework on components. Target is always set by the framework on Routes and Controllers during instantiation, to create the chain of bubbling events.

So aside from the possible unknowing setting of target in user-land, components will never bubble on send. That's what sendAction is for.

If you're still in a spelunking mood, the Controller/Route create call happens here but it's some gnarly code that isn't very easy to follow.

[DingoEatingFuzz]

I get the desire to align with DOM APIs here, but I personally don't like that the singular value is being used to represent a collection.

[johncowen]

Yeah agree here, I almost split it by \n to make it a string and then joined it back up again on the other side. It was also detail.value at one point using CustomEvent. I think the thing that makes this 'ok' for me is the fact that as I'm using ember/javascript there is nothing to be gained in splitting/joining things apart from absolute parity with DOM APIs, I'm not sure there is anything practical to be gained from it (at least the target.value is recognisable and and therefore clear what is going on)

I'll have another look today though.

[DingoEatingFuzz]

(thinking out loud)

Creating and throwing away a Set every time you add an item seems unfortunate, but considering it's public API, doing something where the Set is an internal implementation with an Array as the public API seems tricky when combined with the typical data-down approach.

[johncowen]

I think this was just for unique diff checking. Me personally, I'm not too worried about creating Sets, it's just a native object - I'm not sure if this is majorly any different from creating and throwing away Arrays?

The alternative would be loops and conditionals, Set seems to be the more appropriate tool

[DingoEatingFuzz]

Yeah, maybe I'm worried over nothing.

To clarify, I love the use of Set here for uniquing. It's just the throwing it away on every add that raises a (probably irrelevant) flag in my head.

[DingoEatingFuzz]

This seems out of place. What's the story here?

[johncowen]

Ah yeah, I almost didn't put this in here (see the P.S. in the PR description). It was just easier to fit in here, plus this PR is all about 'phrase-editor amends', which the above fits into.

Basically I was trying to keep the 'Remove' text that is inside the little delete buttons for styling/a11y reasons, it got removed in a previous PR without me realizing and 'Remove' text was then visible. This just rehides it.

[johncowen]

Hey @DingoEatingFuzz

Thanks so much for looking over this (and all of the others 🙏 )

I added few integration tests ontop of this, and then there is the send vs bind thing above, which I don't feel strongly about at all. I put my thoughts above, so just let me know what you think and I can further amend and/or merge, I'll keep this hanging here until I hear from you.

Thanks,

John

[DingoEatingFuzz]

Thank you for adding test coverage! They look good.

I stated my case on bind vs. send, but I think this is good to go either way.

[johncowen]

Hey @DingoEatingFuzz

Great stuff thanks for the nudge with the testing there, and thanks for spending the time to state opposing side on the send vs bind thing, really good discussion. I very nearly almost switched to send but went with bind in the end, thanks for the flexibility there.

Thanks,

John