Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better integration of HOC factories #18

Open
zabojad opened this issue Mar 7, 2016 · 19 comments
Open

Better integration of HOC factories #18

zabojad opened this issue Mar 7, 2016 · 19 comments
Labels

Comments

@zabojad
Copy link

zabojad commented Mar 7, 2016

Using more and more react js libraries like react-redux, react-intl, ... I face more and more the concept of Higher order component factories. This concept consist of wrapping your components to add it more features usually via props. It's done by exposing/exporting the result of a factory function that has been given your component class as parameters: for example here.

Another more complexe example is the ReactRedux.connect factory with can take more params.

While this is already implementable with haxe and haxe-react, I think we could had some metadata to expose directly the result of a HOC factory of component right in the same file as the component implementation itself. Something like:

typedef MyCompProps = {
    // (...)
}

@:hoc("myFactory" /*, other params ? */)
class MyComp extends ReactComponentOf<MyCompProps, Dynamic, Dynamic> {

    // (...)
}

We should also probably allow the @:hoc metada to be added several times on the same Component class...

In ES6, it seems we already have such a metadata feature for HOC factories: here is a es6 version use of the ReactRedux.connect factory.

And with javascript, we can (and usually do so) export the "factored" component in the same class file as the component class itself.

@zabojad
Copy link
Author

zabojad commented Mar 7, 2016

Or... it should be up to the ReactRedux (or any other lib) extern to define its hoc factory metadata itself... That probably preferable as it would maybe allow a better checking from the compiler...

@elsassph
Copy link
Contributor

elsassph commented Mar 7, 2016

Some cases HOCs would difficult to reconcile with Haxe I think.

  • stateless function renderers would be simple enough but the macro generator doesn't support them,
  • old-school mixins mess would be tricky to solve using Haxe typesystem,
  • definition of custom classes (connect I think?) might work if es6 classes can extend Haxe ones. But then it might still be tricky to handle the necessary JS code generation for it.

The first step is to figure what Haxe code should be generated for such cases. Eg. if it's possible to write manually, then it can be generated using a macro - otherwise it's likely that it's not possible.

@zabojad
Copy link
Author

zabojad commented Mar 8, 2016

Hi there!

I do not understand all what you wrote... Maybe because I was not clear myself.

I'll try to sumarize the issue:

The HOC factories actually only inject logic/objects in the component props. No mixin here nor extension nor anything else...

When you do var MyReduxAwareComp = ReactRedux.connect(mapStateToProps)(MyComp), it actually only injects additional data or functions/callbacks in the props of MyComp to give MyReduxAwareComp.

The first step is to figure what Haxe code should be generated for such cases. Eg. if it's possible to write manually, then it can be generated using a macro - otherwise it's likely that it's not possible.

Yep, that makes sense to me.

So, if we stick to the ReactRedux example, here is what I currently do (and it works of course):

MyComp.hx:

typedef MyCompProps {
    data : Array<String>
}
class MyComp extends ReactComponentOf<MyCompProps, Dynamic, Dynamic> {

    static function mapStateToProps(state : Dynamic) {
        return {
            data: state != null ? state.data : []
        }
    }

    static public function redux() : Dynamic {
        return ReactRedux.connect(mapStateToProps)(MyComp);
    }

    override function render() {
        // (...)
    }
}

App.hx

class App {
    static public function main() {

        ReactDOM.render(jsx('
                <${MyComp.redux()} />
            '), 

            js.Browser.window.document.body);

    }
}

It would be great to have some nicer syntax that does not require to write this: <${MyComp.redux()} /> but this <MyComp />.

Is it clear to you? How would you see this with a macro/metadata/... ?

Note that the ReactRedux.connect factory is the most complex HOC factory I've seen until now. Usually, it's just a normal function that just takes your comp as parameter like this one:

class MyComp extends ReactComponentOf<Dynamic, Dynamic, Dynamic> {

    static public function intl() : Dynamic {
        return ReactIntl.injectIntl(MyComp);
    }

    // (...)
}

Note also that we need a system that allows "hoc factory chaining" on a single comp (some comp need to be passed through several different HOC factories to let them get features from several libs).

@elsassph
Copy link
Contributor

elsassph commented Mar 8, 2016

I hope you realize that <${MyComp.redux()} /> (besides the syntax) is a terrible thing to do: the ReactRedux.connect function creates a whole new class extending MyComp. See http://github.com/reactjs/react-redux/blob/master/src/components/connect.js
You would end up creating a whole new class every time your render function runs.

That's the reason why the recommended pattern in JS is to completely replace MyComp with the new class that connect creates. But this pattern would be very difficult to use in Haxe because of the way classes are generated in some cases.

I didn't check what ReactIntl.injectIntl does but I've seen an example where they wrap a "pure render" function (stateless component) and you are suggesting that it could wrap also a class like ReactRedux.connect does.

Finally for the sake of exhaustivity, I've seen HOCs adding functions to the class prototype instead of extending the component with a new class. This is also pretty hard to support in typed languages.

A pattern that seem to work is:

class MyComp {
    static public var Connect = ReactRedux.connect(mapStateToProps)(MyComp);
    ...
}

Which you would use as: <MyComp.Connect/>.

That is nicely compatible with Haxe and easy to write manually. Potentially you could come up with a macro generating such Connect property.

@zabojad
Copy link
Author

zabojad commented Mar 9, 2016

I hope you realize that <${MyComp.redux()} /> (besides the syntax) is a terrible thing to do: the ReactRedux.connect function creates a whole new class extending MyComp. See http://github.com/reactjs/react-redux/blob/master/src/components/connect.js
You would end up creating a whole new class every time your render function runs.

Gosh! Thanks for pointing this me out :s!

ReactIntl.injectIntl() also returns a wrapping component... I believe it will be the case most of the time with HOC factories...

Finally for the sake of exhaustivity, I've seen HOCs adding functions to the class prototype instead of extending the component with a new class. This is also pretty hard to support in typed languages.

I have not seen that yet. Also is this compatible with ES6? I haven't learnt ES6 yet but given that mixins are not possible with ES6, I would suspect this way of designing HOC factories would not work with ES6... If that's the case, we could consider this use case of HOC factories to be invalid and thus not support it...

A pattern that seem to work is:

class MyComp {
static public var Connect = ReactRedux.connect(mapStateToProps)(MyComp);
...
}
Which you would use as: <MyComp.Connect/>.

That is nicely compatible with Haxe and easy to write manually. Potentially you could come up with a macro generating such Connect property.

Yep, I agree it's the simplest and cleanest way so far... If it was just to generate this and nothing else, I think no macro is needed here...

However, maybe we could think of a macro that would:

  • generates this above,
  • extends the MyComp original props with the ones added by Redux (in this example)

So that in only one statement (a metadata actually) we enforce the Redux additional props to the MyComp props and "connect" it...

Or maybe that's not a good idea and it has not such a great value that it would require the pain of working on a macro for that...

What do you think?

@elsassph
Copy link
Contributor

elsassph commented Mar 9, 2016

You can still modify the prototype in ES6, and do mixins "manually". There just isn't a way using the class Foo extends Bar syntaxic sugar.

extends the MyComp original props with the ones added by Redux (in this example)

How do you know which props to add? I think you should just define those props as expected by your component even if it wasn't connected through redux.

Generally you want to avoid defining new types - it's badly handled by IDEs unless they are indirectly accessed.

@fullofcaffeine
Copy link

@elsassph Maybe with the help of a custom js generator it'd be possible? It'd be pretty hardcore to do and maintain though. Not sure if worth it.

@elsassph
Copy link
Contributor

elsassph commented Apr 2, 2016

A custom JS generator would make the redux MyComp = connect(mapStateToProps)(MyComp) pattern possible (but a pain to do and maintain) but I believe a better thing to do would be to look into converting this connect function to Haxe semantics. eg. I believe a macro could effectively replace it.

@kevinresol
Copy link
Contributor

kevinresol commented Oct 26, 2016

I successfully use react-redux's connect() with the following code

class VisibleTodoList {
    static function mapStateToProps(state) {
        return {...}
    }

    static function mapDispatchToProps(dispatch) {
        return {...}
    }

    static function __init__()
        // kind of hack to assign something to a class, but works
        untyped VisibleTodoList = js.Lib.require('react-redux').connect(mapStateToProps, mapDispatchToProps)(TodoList);

}

class TodoList extends ReactComponent {
    override function render() {
        return jsx('...');
    }
}

@kevinresol
Copy link
Contributor

You can further improve it by writing some macros to insert the __init__ function like:

@:build(ReactReduxMacro.connect(TodoList))
class VisibleTodoList {
    static function mapStateToProps(state) {
        return {...}
    }

    static function mapDispatchToProps(dispatch) {
        return {...}
    }
}

class ReactReduxMacro {
    public static function connect(e:Expr) {
        var cls = Context.getLocalClass().get();
        return Context.getBuildFields().concat([{
            access: [AStatic],
            name: '__init__',
            kind: FFun({
                args: [],
                expr: macro untyped $i{cls.name} = js.Lib.require('react-redux').connect(mapStateToProps, mapDispatchToProps)($e),
                ret: null,
            }),
            pos: Context.currentPos(),
        }]);
    }
}

@elsassph
Copy link
Contributor

This is starting to be clever, but that isn't really a scalable pattern of you want to heavily use HOCs.

@Glidias
Copy link

Glidias commented Jan 15, 2017

What are the possible issues with this approach, seems to be fine if the end user is given the choice to either use the lower order component vs the higher order one in a Render.

@fullofcaffeine
Copy link

Any updates on haxe-react HOC patterns? HOCs are not uncommon in a React codebase so having some utility or clear pattern to straightforwardly apply HOCs from within haxe-react would be nice. For now, the best examples of what @kevinresol has shown above.

@elsassph
Copy link
Contributor

elsassph commented May 22, 2020

Current recommended approach would be using @:jsxStatic:

@:jsxStatic(wrapped)
class VisibleTodoList {

    static function mapStateToProps(state) {
        return {...}
    }
    static function mapDispatchToProps(dispatch) {
        return {...}
    }

   static final wrapped = js.Lib.require('react-redux').connect(TodoList, mapStateToProps, mapDispatchToProps);
}

PS: it's better BTW to declare mapDispatchToProps as a static object.

@kLabz
Copy link
Contributor

kLabz commented May 22, 2020

There are with react-next https://github.com/kLabz/haxe-react/blob/next/doc/wrapping-with-hoc.md
Should be (mostly?) applicable to this lib as well, actually (see #102)

@elsassph
Copy link
Contributor

Oh actually @wrap. Undocumented so might as well not exist :D

@kLabz
Copy link
Contributor

kLabz commented May 23, 2020

Copying everything from the link above up to @:publicProps(TProps) part should cover basic usage for haxe-react.

@elsassph
Copy link
Contributor

Reorganised and completed the documentation about it - check the readme :)

@kLabz
Copy link
Contributor

kLabz commented May 24, 2020

Nice, readme looks a lot better like that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants