Refactor Button and ButtonGroup to Glimmer components

[simonihmig]

Proof of concept of how easy/difficult it would be to refactor components:

• https://github.com/ember-codemods/ember-tracked-properties-codemod helped a bit
• used https://github.com/jkusa/ember-arg-types at first, but later decided to move most usages into the template (named args, custom helpers instead of CPs), and a custom, very simple @args decorator
• took a few hours, but was ok
• tests for <BsButtonGroup> and <BsDropdown> are expectedly failing, as they access private properties of the button, which does not work anymore
• tests for <BsButton> should be passing, without any changes. Does that make us confident enough that we can refactor component by component in minor version updates, without breaking changes? Or should we do a single major version release with all component refactored? 🤔

[jelhan]

This looks good to me.

used https://github.com/jkusa/ember-arg-types at first, but later decided to move most usages into the template (named args, custom helpers instead of CPs), and a custom, very simple @args decorator

I'm not sure if the @args decorator is needed at all. It's used at two placed in this pull request. Both could be refactored to named arguments. Such an alias with default value is only needed if the argument is used a lot in JavaScript file itself in my opinion.

tests for <BsButton> should be passing, without any changes. Does that make us confident enough that we can refactor component by component in minor version updates, without breaking changes? Or should we do a single major version release with all component refactored? thinking

In general: Yes. But there are edge cases. E.g. refactor <BsForm> and <BsForm::Element> to Glimmer Components would break the plugin API for validation support. Not sure if there are more of these cases. But at least these two must be changed as part of a major release.

[simonihmig]

I finally was able to get back to this:

• rebased
• refactored also ButtonGroup and Dropdown/Button
• all tests pass now (without having touched them, yeah!)
• CI is failing for Ember Canary, but for some form tests, so apparently unrelated

If we are confident enough to release this - and other future Glimmer refactorings - as a regular non-breaking update (seems so judging by not having touched any tests, but who knows if we miss some edge cases?) in the 4.x cycle (maybe with a pre-release like 4.1.0-beta?), then I should bring back the subclassing deprecations as discussed here

@jelhan thoughts?

[jelhan]

Thanks for following up here. Awesome results. 👏

If we are confident enough to release this - and other future Glimmer refactorings - as a regular non-breaking update in the 4.x cycle

I think we should do! If we notice that we have broken something that we can not fix within @glimmer/component we can still revert and park that refactoring until v5 lands. Incrementally moving to a Octane worl without requiring a major release for each step seems to be the ember way. 😜

[jelhan]

The documentation for the removed private properties could be removed as well. Or did I missed something? It's the same for buttenGroupType as well.

[simonihmig]

Those arguments are still passed, from the ButtonGroup component that yields the button. They are just not used on the component class, but as named arguments in the template. I thought we can still leave the doc blocks to document what arguments the component expects.

[jelhan]

It seems as if we can deprecate value argument. The same can be achieved with {{fn}} helper. Not related to this merge request but the refactoring makes it more visible. Or did I missed something?

[simonihmig]

Yeah, so the ButtonGroup's version of a button needs a value, as that determines the value of the group (e.g. an array of all selected button values). The base button does not really need a value, I agree. I think we can refactor that eventually, but only separately as a breaking change!

[jelhan]

I'm still thinking that this is better done by currying the arguments using {{fn}} helper.

For the <Button group::Button> it's not that obvious cause how it extends from <BsButton>. Instead of invoking the <BsButton> component in a template it extends from its class. To be honest I don't like that pattern.

I think the <BsButtonGroup::Button> should be refactored to something like this:

<BsButton
  @active={{this.isActive}}
  @onClick={{fn @onClick @value}}
  @type={{@type}}
  @buttonType={{@buttonType}}
  ...attributes
  as |button|
>
  {{yield button}}
</BsButton>

[jelhan]

Don't get me wrong. This is nothing which is blocking this PR. I will see if I have some time in the next days to provide a PR showcasing such a refactoring.

[simonihmig]

@jelhan thanks for the 2nd review! Some how I missed this...

I think I addressed your feedback, thanks for that! Rebased again, after some merge conflicts (#1221). And fixed the new submitButton feature, which was broken by these changes. Hope everything turns green now! 🤞

[simonihmig]

✅ All green! 🚀

I'll do another bugfix release first before merging this, so we get that fix out without coupling that to eventually (unforeseen) breaking changes.

But after that, I'll like to get this merged, if @jelhan you have nothing left, to not need another rebase! Also I have another branch lined up already that refactored BsAlert, but builds on top of this, so I'll wait to get this merged before opening the PR...

[simonihmig]

I'll do another bugfix release first before merging this

Done! :)

[jelhan]

I'll do another bugfix release first before merging this, so we get that fix out without coupling that to eventually (unforeseen) breaking changes.

I noticed that I missed to update include / exclude feature when implementing form submit button feature. Provided a fix in #1223.

But after that, I'll like to get this merged, if @jelhan you have nothing left, to not need another rebase! Also I have another branch lined up already that refactored BsAlert, but builds on top of this, so I'll wait to get this merged before opening the PR...

Awesome. 👏 Let's ship it.

Are you planning to bundle multiple refactored to @glimmer/component in the same minor? Or do you want to ship each one as soon as it's ready?

[simonihmig]

Are you planning to bundle multiple refactored to @glimmer/component in the same minor? Or do you want to ship each one as soon as it's ready?

As I said, I have BsAlert already lined up, so would at least add that too.

Though about maybe releasing a 4.2.0-beta version for people to test this, before a stable v4.2.0? In that case, do you think it makes sense to bundle more refactorings into it, so to have less work testing these kind of prereleases?

[jelhan]

Though about maybe releasing a 4.2.0-beta version for people to test this, before a stable v4.2.0? In that case, do you think it makes sense to bundle more refactorings into it, so to have less work testing these kind of prereleases?

Releasing directly as v4.2.0 would be fine for me as well. We can still revert if needed. But in both cases I would release as soon as possible. Better catch regressions not covered by our testing suite as early as possible to not waste time.