feat(react): add connect React HOC

[tlaundal]

A higher order component for connecting react components to streams.
Builds upon the useStream hook, and is intended for use in class based components that can't use the hook themselves.

Rigid testing with react-test-renderer, which renders the components without a DOM. Have also chosen to not write any tsx, so we won't have to deal with more tooling.

[holographic-principle]

Isn't the precedence in the current connect (from streamUtils) the opposite? (...value, ...props)

[anton164]

Nice catch @holographic-principle.

Personally, I don't see a strong case for why "props" or "streamedProps" should take precedence 🤷‍♂ Did you switch these around for any particular reason @tlaundal ?

In general, I can't think of a case where having the same key defined in "props" and "streamedProps" is a good pattern as I think this could cause a lot of confusion. Perhaps we could add a warning in dev mode?

[tlaundal]

So actually, this kind of shadowing is actually caught by TS because of the types (this is what the Omit does), so I don't really think this is a problem.

I can change the order to be the same as in ardoq-front, just to be on the safe side, but I don't think we should document or test this, as it is explicitly forbidden by the types.

EDIT: Actually, It should be the way I have already defined it, and it should be tested.
Take this example:

const WrappedComponent = (props: { msg: string, num: number }) => (<p>{msg}</p>);
const HOComponent = connect(WrappedComponent, of({ msg: "from stream" }));

const illegal = <HOComponent num={1} msg="direct overshadowing gives type error" />;

const props = { num: 2, msg: "indirect overshadowing does actually not give error" };
const legal = <HOComponent {...props} />;

The illagal line here is the behaviour we wish for. legal is a work around for the type error, but I think it makes sense to follow the idea of the Omit typing:

Only properties which are not supplied by the stream can be supplied manually

I will add a test and a sentence documenting it, but I don't see the need for a dev mode warning.

[anton164]

Ok, good 👍

[chriskr]

LGTM

[anton164]

I think I remember you mentioned that this would solve connectWithProps as well, do you have an example of that or will that be a next step?

[holographic-principle]

I'm purely guessing here, but I think Tobias' intention might have been to replace connectWithProps with the HoC from this PR. That would explain the order in which props vs streamedProps are spread. However this is (potentially) not backwards-compatible with connect.

[tlaundal]

connectWithProps use properties as a part of the stream (usually filtering). The useStream hook can be used in it's place.
This HOC cannot replace connectWithProps because the properties are not available when the stream is initialized.

I'll create a PR in ardoq-front where I replace connectWithProps with useStream today.

[anton164]

Looks good! 👍

A warning in dev mode for key collision in "props" and "streamedProps" would be nice :)

[tlaundal]

I have clearified in the documentation and with tests the precendence of passed/forwarded props and streamed props. Will merge in about half an hour.