GWTReact / gwt-react

GWT Java bindings for React
MIT License
91 stars 17 forks source link

Implementing the builder pattern for Props classes requires too much ceremony and does not play well with inheritance #16

Closed ivmarkov closed 6 years ago

ivmarkov commented 7 years ago

Creating a new custom component' Props class means we have to - for each custom property:

Furthermore, the Props builder pattern does not play well with inheritance.

Imagine implementing a custom button (or binding a JS library that has implements e.g. a Material Design button). The custom button component would essentially support all BtnProps, plus some additional of its own (say a custom "raised" property). If we just inherit from BtnProps and implement the ceremony of the builder pattern from above, this results in the following inconvenience:

new MaterialBtnProps()
.id("blah")
.raised(); // WRONG: won't compile because the id() fluent setter from above returns BtnProps, not MaterialBtnProps!

This would compile, but (a) we then ask the user to be aware of the inheritance hierarchy and (b) requires an ugly upcast:

(MaterialBtnProps)new MaterialBtnProps()
.raised() 
.id("blah");

Sure, we can parameterize BtnProps by its descendant like this:

class BtnProps<P extends BtnProps> {
    @JsOverlay public id(String id) {
        setId(id);
        return thiz();
    }

    @JsOverlay private thiz() {
        return (P)this;
    }
}

... but this means even more ceremony!

Other alternatives? How about the "double curly braces trick" (inspiration came from here: https://github.com/cincheo/jsweet-examples-react/blob/master/src/main/java/examples/SimpleExample1.java). Essentially, we give up on the builder pattern completely, model Props classes with simple public fields, and then - when instantiating Props, we use anonymous inner Props classes with double curly braces to minimize the initialization noise, like this:

@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object")
class BtnProps {
    public String id;
    ...
}

@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object")
class MaterialBtnProps extends BtnProps {
    public boolean raised;
}

Usage of MaterialBtnProps:

React.createElement(
    MaterialButton.TYPE,
    new MaterialBtnProps() {{
        raised = true;
        id = "blah";
    }});

... which can be of course further shortened by hiding the React.createElement call behind some terser syntax.

pstockley commented 7 years ago

For sure, the builder pattern is far from ideal. Does the the anonymous inner class approach even work with GWT? There was talk at one time to turn these into object literals but it didn't make it into 2.8. I would be worried about the code size/performance implications of creating lots of inner classes.

To be honest, I don't like the whole function syntax at all and it does pay a penalty in terms of performance compared to native JS use of React. I have been contemplating writing an annotation processor that would allow me to write JSX and then turn this into optimized code. With intellij language injections you could even get code completion/syntax checking.

ivmarkov commented 7 years ago

For sure, the builder pattern is far from ideal. Does the the anonymous inner class approach even work with GWT?

Yes. And even if - at some point - React starts to complain for not really receiving an object literal, we can always apply the new JsPlainObj().merge(<anonymous-inner-class>) trick inside React.createElement to make sure that React sees only plain object literals.

There was talk at one time to turn these into object literals but it didn't make it into 2.8. I would be worried about the code size/performance implications of creating lots of inner classes.

But they are not that many. Basically, one per component instantiation (React.createElement). Furthermore, the creation of the anonymous class is a penalty you pay once only, during the module initialization. new-ing anonymous inner classes shouldn't be much more expensive than creating object literals. At worst, a couple more assignments to setup the prototype chain.

To be honest, I don't like the whole function syntax at all and it does pay a penalty in terms of performance compared to native JS use of React.

Is it? Your JSX syntax translates 1:1 exactly to nested function calls to React.createElement. No magic here.

I have been contemplating writing an annotation processor that would allow me to write JSX and then turn this into optimized code. With intellij language injections you could even get code completion/syntax checking.

Yeah. Except that your JSX syntax (unless I'm missing something?) will be embedded in a string, which will live inside your annotation. Just imagine the noise: JSX expressions (potentially containing themselves Java or JavaScript expressions) embedded in single-line strings, concatenated with Java's + operator.

Unless you can put the JSX syntax directly in the Java method body, that is. But then we are talking about extending Java-the-language, which is a whole topic by itself. Or do you plan to take the JSNI approach and put the JSX in specially crafted comments?

Regardless, the JSX idea makes an even stronger case for less ceremony when modeling the Props classes. Because with JSX, you don't need that ceremony at all, as the props objects' setup will be hidden inside the JSX syntax, and can be - during JSX-to-Java translation - translated to

Props props = new Props();
props.prop1 = "blah";
props.prop2 = "foo";
...
React.createElement(props, ...);
ivmarkov commented 7 years ago

If you are still unconvinced that the performance penalty of anonymous inner classes is negligible, take a look here: https://github.com/facebook/react/blob/15-stable/src/isomorphic/classic/element/ReactElement.js

... specifically, line 200 and onwards. A new props object is created each time you call React.createElement(), and a shallow copy is made from the anonymous inner class into the props object. For the children, the processing is even heavier (new array, etc.).

pstockley commented 6 years ago

To reduce some of the boiler plate and make the props easier to work with the static initializer trick I have removed the setters/getters and replaced them with properties. Originally I wanted to do that but there were issues with very early versions of GWT2.8

ivmarkov commented 6 years ago

Sounds good to me!