Copy custom group properties in group-utils copyGroup function

[lukasnys]

Why?

In our use case of ember-power-select we add custom properties onto the groups to conditionally render those groups in a different way (in the @groupComponent that we pass to PowerSelectMultiple).

However, because the copyGroup function strips those custom properties, our styling is broken as soon as a user enters a search term.

What?

Change the copyGroup function to include all fields.

[mkszepp]

Not sure if this works in your case, but we have a @extra paramenter to pass additional option to any place... maybe this helps you already to fix your issue?

If it isn't working with @extra, can you add any test for this changes, so there is always save for future updates

[lukasnys]

@mkszepp thanks for the suggestion! I don't believe the @extra works for our use case though.
As we'd need to pass the the custom properties as @extra to the @groupComponent and those custom properties can be unique per group object.

How do you suggest I should test these changes? Should I export the copyGroup function and then write a unit test for it?

EDIT: I took a stab at writing tests for this

[mkszepp]

@lukasnys for test i would like you add a test like a real usecase... for example creating a component in which you are passing this property... so we don't need to export the function right?
Its always better to make real example...

With @extra you need maybe to create the object with more dimensions to get run, but it works without override of any component... Passing custom parameter works atm fine, but when you will move any day to glint (which will be recommended in near future to every app and is also already usabel) your use-case could be more configuration inside your app, because glint validates if every parameter you pass, exists on component and has correct type. So you need also to override component signature... inside your app to get run.

[lukasnys]

Hey @mkszepp, that makes sense! I removed the unit tests and added an integration test for my change.

[lukasnys]

I also created #1844 where I add the same test to showcase that it's currently failing.

[mkszepp]

Thanks looks already good! Can you add als a test for single select? Than i think we test everything

Can you fix also the embroider test?

[lukasnys]

Added the single select test case and fixed the Embroider test!

[mkszepp]

Thank you! LGTM!

[mkszepp]

Ah one thing.. for TS i think we need to extend the Group with [key: string]: unknown otherwise its not possibile to use inside TS project... can you maybe add also this one?

Line 120 in group-utils

[lukasnys]

Will do 👀

In the implementation of the copyGroup function, do you think it makes sense to keep the explicit check for the disabled property. Or do you think I can remove it since we have the ...group?

Implementation below as reference

function copyGroup(group: Group, suboptions: any[]): Group {
  const groupCopy: Group = { ...group, options: suboptions };
  if (Object.prototype.hasOwnProperty.call(group, 'disabled')) {
    groupCopy.disabled = group.disabled;
  }
  return groupCopy;
}

[mkszepp]

Will do 👀

In the implementation of the copyGroup function, do you think it makes sense to keep the explicit check for the disabled property. Or do you think I can remove it since we have the ...group?

Implementation below as reference

function copyGroup(group: Group, suboptions: any[]): Group {
  const groupCopy: Group = { ...group, options: suboptions };
  if (Object.prototype.hasOwnProperty.call(group, 'disabled')) {
    groupCopy.disabled = group.disabled;
  }
  return groupCopy;
}

I think, we can hold like it is, so the autocomplete is better

[lukasnys]

Added [key: string]: unknown to the Group interface 👍

[mkszepp]

Thank you... i will release it on begin of next week

[lukasnys]

Thanks! 🎉