refactor(types): to refine types for each adapter

[emmenko]

Summary

The main intent with this PR is to refine the adapter types.

At the moment, there is one AdapterArgs type that is used by all the adapters. This results in adapter specific props that cannot be properly represented (e.g. clientSideId).

Description

Ideally, I would expect the type system to properly check the adapter specific props when the related adapter is used.

To achieve this, we can expose a union of specific adapter types. Then each adapter implements a type guard to properly validate the adapter type props.

In order to make this work properly, we also need to extract the "event handlers" out of the adapter args, as they should not be exposed. An attempt to fix that was made in #892

While refactoring that part of the codebase, I noticed that the memoization functions are not necessary anymore, so I removed them. This might also fix the issues we are seeing with memoization.

[emmenko]

Notice how the configure function separates the args (provided by the user) from the event handlers

[emmenko]

I took the liberty to refactor this to a function component, as it makes the refactoring simpler

[emmenko]

This is part of the SplitioAdapterArgs now

[emmenko]

Giving this a bit more thought, we also need to “tell” the configure components which adapter (type) we are using, and thus having a type check top to bottom.
Will push a commit tomorrow

[emmenko]

To clarify a bit more what the expectations are: when I render the ConfigureFlopFlip component, I expect the adapterArgs to be correctly validated according to the given adapter.

[emmenko]

Alright now things are more or less working as I wanted from a consumer perspective.

To achieve this, I had to refactor a bit the adapters and opted to use interfaces, which makes type checking easier. It also provides a nice interface for a base adapter. It comes with a bit more boilerplate though, but I personally don't mind much.
As a follow up, we can also move the adapter initialization logic into the adapter class constructor, to have a full encapsulation of the adapter.

---

At this point I achieved what I was aiming for, at least from a type check perspective from the consumer side. Maybe we can simplify one thing or two, but I couldn't find any simpler approach.
Let me know if this sort of changes are inline with your vision, or if this all thing makes sense to you. Otherwise we can also look into other approaches.

Thanks

[emmenko]

@tdeekens is it normal that the github actions only run on my fork and not here on this PR? 🤔

[tdeekens]

Thanks for contributing so much lately. The idea is really nice! Maybe we need to add an event to when the actions run. I am still not sure when to use push and pull_request.

[emmenko]

@tdeekens is it normal that the github actions only run on my fork and not here on this PR?

Fixed in the last commit: 439a5eb

See:

• Github actions don't work for pull requests created that involve a fork. WordPress/gutenberg#17324 (comment)
• https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows#pull-request-events-for-forked-repositories

[tdeekens]

Awesome! Got another chance to review it in detail. Looks really great. Totally worth it 😄. Two minor notes:

1. We should release this as breaking (commented somewhere)
2. The TypeScript funk with interaces and implements moves away more from standard JavaScript than I'd hoped we'd ever have to when adopting TS

[tdeekens]

This will be breaking to flopflip as we require only 16.3 as a peer dependency of react so far. Hooks are 16.8. It's totally fine by me and about time in a way. However, wanted to note :)

[emmenko]

I see. If you want, we can still be backwards compatible.

If you're ok with breaking changes, we should probably do some more "cleanup":

• remove depreacted lifecycle methods
• remove APIs that might not be necessary anymore
• rewrite components to functional components
• use hooks, rewrite HOCs using hooks
• ??

We can also work on a different base branch.

So if you want to proceed in this direction, I can refactor this PR to avoid using hooks, we can then merge this and start working on the breaking changes on a different base branch.

[tdeekens]

To me this PR as it stands is totally worth it. We give and take a bit. Give improved typings on adapters and take away from React/redux support. Things you've mentioned above are also on my list but all relate on the same dropping of underlying React support to my understanding.

Having said that I think we can break here once. I actually like breaking releases without much else. Just a breaking change and maybe a small improvement for bait 😄.

In follow ups we can split out:

1. HoCs to build on hooks
2. Remove deprecated lifecyle (componentWillReceiveProps)
   • This can get a bit hairy as it's on react/configure which is a bit the brain of the library
3. Rewrite anything we can to functional components

Also almost in that order as one enables the other. I just haven't got to it as it's mostly maintainance work and it's odd when your cute little puppy Open Source project transitions into a 80% maintainance hole 🐼. I'd obviously appriciate any help.

As a last step we can then have a look if some APIs should just go away. Candidates for me are render in favor of children just like formik with a deprecation.

What do you think?

[tdeekens]

I started a board and created issues: https://github.com/tdeekens/flopflip/projects/1

[emmenko]

We should release this as breaking (commented somewhere)

Ah I see...we can still be backwards compatible if you want to.

The TypeScript funk with interaces and implements moves away more from standard JavaScript than I'd hoped we'd ever have to when adopting TS

Not necessarily. Even without types, I think you can still implement the adapters as classes, since they have a similar API. It's also an implementation detail, as the class is not exposed directly to the package.

[tdeekens]

adapters as classes, since they have a similar API. It's also an implementation detail, as the class is not

Yeah. You would just have to change some minor keyword use. So all cool 🤓.

we can keep it backwards compatible

I don‘t think we have to. Hooks have been out for about a year. Redux with it (also a breaking peer dep) is also stable. I also do not have many numbers on version usages. Only that more use the redux version than without. That said I think we can release breaking and see if something comes back and then reflect or add backwards compatibility. WDYT?

[emmenko]

Sure sounds good to me. The only question is if you want to include more (planned) breaking changes or just this one.

[tdeekens]

No I haven’t planned any other breaking changes. I will release it some time next week on the train ride back.