search

Yomguithereal / baobab-react

React integration for Baobab.
MIT License
309 stars 38 forks source link

React 17 context API & hook #133

Closed JosephClay closed 5 years ago

JosephClay commented 5 years ago

First steps of updating the library to remove legacy getChildContext API

Next steps:

@Yomguithereal does this sound like a good plan?

JosephClay commented 5 years ago

Refactoring to componentDidUpdate required a deep-equality check to keep from going into an infinite loop: https://www.npmjs.com/package/deep-equal

JosephClay commented 5 years ago

Hook useBranch and hook tests have been added, mimicking higher-order's tests

Yomguithereal commented 5 years ago

Hello @JosephClay

remove mixins?

Probably not. I still know people using them and I feel it would be sad to leave them behind (even by bumping a major version).

Did you write docs for the hooks?

@arnaudmolo can I ask you for your opinion on those hooks?

JosephClay commented 5 years ago

@Yomguithereal Haven't created documentation yet

Would this be the way you like the hook to be exposed?

import { useBranch } from 'baobab-react/higher-order';
Yomguithereal commented 5 years ago

Maybe more:

import { useBranch } from 'baobab-react/hooks';

Also should we move to new context APIs?

JosephClay commented 5 years ago

The hook can't work on its own, it still needs a rooted top-level component to consume the Context. I can note that in the documentation

New context APIs were the first things added to remove getChildContext

arnaudmolo commented 5 years ago

The hook looks ok, as you both stated we would need it in the doc.

I'm just wondering, what will be the price of replacing ComposedComponent & co with functionnal components ? I'm guessing it could be done with hooks, bu I don't really know baobab's code base.

JosephClay commented 5 years ago

@Yomguithereal @arnaudmolo I've added documentation for hooks, based off higher-order. In fact, with the refactor to using the new Context API, higher-order and hooks are interoperable!

I've added the hook useRoot to be consistent

Added dispatch to the useBranch hook for parity with higher-order and added action tests

Yomguithereal commented 5 years ago

Seems good to me. I just have a remark on the docs I listed herebefore. @arnaudmolo everything's good for you?

arnaudmolo commented 5 years ago

@Yomguithereal 👍

Yomguithereal commented 5 years ago

I am going to merge this. @JosephClay should this be a v4 or can this be a safe v3.1?

JosephClay commented 5 years ago

@Yomguithereal I believe a v4 would be in order, as the Context API requires React ^16.x. Has the potential to break projects using higher-order with an older version of React

Yomguithereal commented 5 years ago

Fair enough. Let's go then :)

Yomguithereal commented 5 years ago

v4.0.0 is now live on npm. Thanks @JosephClay.