Adding unit tests for useCompositeState to Composite component

[andrewhayward]

What?

This PR adds unit tests to validate useCompositeState behaviour.

Why?

In order to ease the migration away from reakit, we need to maintain the current public Composite API. reakit exposes a useCompositeState function, which ariakit doesn't directly support, so we need to port this to the new Composite implementation.

To ensure this work maintains existing behaviour, we need to have unit tests in place.

How?

Tests are written against each prop and action provided by Reakit's useCompositeState implementation.

Additionally, tests are written for common use-cases to validate interaction and developer expectation.

Testing Instructions

• Confirm that test coverage is complete
• Ensure that all tests run

[ciampo]

Thank you for working on this!

I was wondering if, instead of meticulously testing the hook's arguments and return values, a better approach would be to write tests from the point of view of the consumer of the component — eg

const TestExample = () => {
  const state = useCompositeState();

  return (
    <Composite {...state}>
      <CompositeItem>Item 1</CompositeItem>
      <CompositeItem>Item 2</CompositeItem>
      <CompositeItem>Item 3</CompositeItem>
    </Composite>
  );
};

// test the most important features of composite, eg:
// - default roles and semantics
// - default tkeyboard navigation
// - orientation
// - two-dimensional

I believe that's a better fit for the needs of this refactor — making sure that we reduce as much as possible the disruption to consumers of the component. What do you think?

[diegohaz]

Fantastic job! Thanks a lot!

I've got a question, though. Does it make sense to re-implement those unstable_ props? They're not even a part of the Reakit public API (which means Reakit could have changed them in patch versions).

When these components were re-exported in @wordpress/components for internal use, they were not expected to preserve their API forever.

I know practices have shifted. But, with that in mind, I'm wondering if it might be better to provide a minimal API for now, focusing on consumer usage, and only re-implement those obscure features if problems arise.

[andrewhayward]

I was wondering if, instead of meticulously testing the hook's arguments and return values, a better approach would be to write tests from the point of view of the consumer...

I've added usage tests to cover the primary modifiers (orientation/loop/wrap/shift).

The API tests may not be worth keeping in the long term, if only from an overhead perspective, but I think I'd be more confident including them for now, at least until we're done with the migration.

I'm wondering if it might be better to provide a minimal API for now, focusing on consumer usage, and only re-implement those obscure features if problems arise.

I think I'd like to leave that conversation for the actual implementation work – as and when we decide that something doesn't need to be included, it'll be easy enough to remove the specific test that goes with it.

[ciampo]

I'm personally not sure about the current approach in this PR of testing the exact implementation and signature of the useCompositeState component.

As discussed previously, we're aiming for a partial backward compatibility layer that supports the most common way the components are consumed and the most common options. Therefore, I would avoid defining types, and I would not add tests around the hook itself. Instead, I would write tests that simulate how a consumer of the component would use the useCompositeState hook together with the rest of the Composite components (see previous suggestion).

Potentially, we would then be able to add best-effort backward compatibility by doing something like

function useCompositeState(props) {
  const store = useCompositeStore( translateStateArgsToStoreArgs( props ) );
  return { store };
}

which should just continue working for consumers adopting the pattern:

const state = useCompositeState();

return ( <Composite { ...state }> {/* children */} </Composite> );

Although I'd like to hear opinions from other folks too, cc @diegohaz @tyxla @mirka

[andrewhayward]

...we're aiming for a partial backward compatibility layer that supports the most common way the components are consumed and the most common options.

I'm not entirely sure how unit tests around (at least some of) the API itself negate this, but I'd be happy to hear your thoughts.

As I said above, I don't think all of the functionality has to be replicated, and I don't think all of the tests (or the types) have to remain in place. But having (some) tests for the API allow us to verify the most common options.

Instead, I would write tests that simulate how a consumer of the component would use the useCompositeState hook together with the rest of the Composite components

FWIW, I have written tests that do this already (see 53a5dfb4, 6ea08c60, 5e45b4dc, and a79af37c). Unless I'm missing something in your suggestion, in which case please feel free to clarify.

I'm quite happy to limit the scope of this as much or as little as required, and I entirely agree with the sentiment that "it might be better to provide a minimal API for now, focusing on consumer usage". I was just going to leave the decision of what constitutes the most common options for when we actually do the implementation (out of scope for this issue).

[andrewhayward]

Dropped the typing, removed the full API coverage, and added in support for testing the main ways that Reakit composite components can consume state props.

[github-actions]

Flaky tests detected in 908d6a2.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7228950917
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js

[ciampo]

Thank you for working on this, @andrewhayward ! I left a couple more comments, but his is definitely moving in the direction that was discussed.

[ciampo]

I initially thought that the "custom props" scenario involved passing, as props, the same props that could be passed to the useCompositeState hook — if that was the case, implementing backward compatibility would be potentially be as easy as doing

const Composite = ( {
  // Destructure all supported "custom props" 
  initialStateProp1,
  initialStateProp2,
  // state and store objects
  state,
  store,
  ...restProps, 
} ) => {
  const internalState = useCompositeState( { initialStateProp1, initialStateProp2 } );

  return (
    <Composite
      store={ store ?? state.store ?? internalState }
      { ...restProps }
    />
  );
}

Instead, it looks like those "custom props" are the same as the return value of the useCompositeState hook. If that's the case, I'm not sure how we'll be able to support backward-compat for this scenario without writing a lot of custom code, and we may need to drop this scenario in the test

[andrewhayward]

Yeah, essentially useCompositeState returns a collection of functions and values that components can use to read and update the shared composite state.

These props are returned by the state hook. You can spread them into this component ({...state}) or pass them separately. You can also provide these props from your own state logic.

So there's actually nothing special about what useCompositeState returns, beyond it being the easiest way to make sure the state is properly shared. Any or all of the returned props could be created outside of that call.

[ciampo]

Got it. So, it looks like we wouldn't be able to guarantee backward compatibility on this front, unless we added explicit code supporting each of these props on Composite ?

[andrewhayward]

Yes. Fortunately there aren't that many, so it wouldn't be an insurmountable challenge. "Worth doing?" is of course a different question!

[ciampo]

I say we don't support this scenario, since we agreed that (at least in the first iteration of adding backwards-compat) we wouldn't add extra code to re-implement reakit functionality, and we would go with a minimal solution instead.

We can probably drop the useCustomProps scenario in the tests too, then.

[ciampo]

Neat!

[ciampo]

LGTM 🚀

Let's move forward and iterate on the tests in follow-ups as necessary.