Skip to content

Conversation

@lkmill
Copy link

@lkmill lkmill commented Apr 5, 2020

To fix #171 and not use any UNSAFE_ methods, i converted the react integration to use hooks.

I haven't updated the tests yet, want to hear whether this is an acceptable change first.

Besides removing the warnings, this also greatly reduces the build file sizes:

Before:

Build "unistore" to .:
        620 B: react.js.gz
        531 B: react.js.br
Build "unistore" to full:
        801 B: react.js.gz
        708 B: react.js.br
        809 B: react.es.js.gz
        717 B: react.es.js.br
        861 B: react.umd.js.gz
        757 B: react.umd.js.br
Build "unistore" to .:
        466 B: react.js.gz
        393 B: react.js.br
Build "unistore" to full:
        682 B: react.js.gz
        592 B: react.js.br
        694 B: react.es.js.gz
        616 B: react.es.js.br
        741 B: react.umd.js.gz
        649 B: react.umd.js.br

With some code golf, eg not using array destructuring, it is quite simple to reduce the size further but the code becomes less readable.

Copy link
Contributor

@Akiyamka Akiyamka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgfm

* @connect( state => ({ foo: state.foo, bar: state.bar }) )
* export class Foo { render({ foo, bar }) { } }
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

return Child => function Wrapper (props) {
const store = useContext(Context);
let [state = mapStateToProps(store ? store.getState() : {}, props), setState] = useState();
boundActions = boundActions || actions ? mapActions(actions, store) : { store };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think useMemo would be a lot better here, this seems way too likely to break

let boundActions;
return Child => function Wrapper (props) {
const store = useContext(Context);
let [state = mapStateToProps(store ? store.getState() : {}, props), setState] = useState();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let [state = mapStateToProps(store ? store.getState() : {}, props), setState] = useState();
let [state, setState] = useState(mapStateToProps(store ? store.getState() : {}, props));

return (Wrapper.prototype = Object.create(Component.prototype)).constructor = Wrapper;

return store.subscribe(update);
}, []);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}, []);
}, [store]);

Might want to at least put the store in here
maybe props as well?

@ForsakenHarmony
Copy link

Super old PR lol 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove depracated in react 16.9 componentWillReceiveProps

3 participants