Introduce GwtReact components based on React's ES6 classes

[ivmarkov]

That's a second attempt - which - compared to #3:

• Has no backwards incompatibility changes
• In the name of simplicity, does not support React contexts (yet). But please see #6
• Has a much smaller change surface

Again, the major benefit of this work is that with ES6 components, the component's constructor will not be called until the component is instantiated by React itself. There is a clear distinction between the component object, and its Java Class<> instance. It is the Class<> instance which is registered when calling React.createElement

The philosophy of this change is to:

• Not to expose an API as public, if it is not. Thus, render(), as well as the component's lifecycle methods (shouldComponentUpdate & friends) are protected rather than public methods. That's also one of the reasons why they are not isolated in a separate Java "lifecycle" interface - that would make them public.
• Favor type safety and enable IDE code completion of the method names - that's why the above component lifecycle methods are exposed as real, existing Java methods that the user can override.

[pstockley]

Thanks, I will take a look. The only issue I have with defining the lifecycle methods on the base class is that this will introduce some overhead i.e. empty methods will be called even if you don't need the lifecycle hook in your component. Practically, I am not sure of the overhead and the impact will probably be different for each lifecycle event.

[ivmarkov]

All the options are imperfect, one way or the other:

1. Current way: pros: very simple to use; type safe; proper methods visibility (protected) cons: potential performance issues
2. Move methods to "lifecycle" interfaces, one interface per method (as you might need to override just one method): pros: type safe, should not have performance issues cons: improper methods visibility (will be public); a bit harder to use (user has to realize he needs to e.g. add implements ComponentWillMount for e.g. comfortable overriding of the componentWillMount() method
3. Use your original idea where methods are not declared anywhere pros: should not have performance issues cons: not type safe, hard to use (user might mistype the method signature)

If you are not happy with (1), perhaps (2) is the way to go? (2) has the additional benefit that if the user does not want to use the lifecycle interfaces, he might skip them alltogether, and just define a method with the proper signature - essentially (3)

[ivmarkov]

Hmmmmmmm. I did some more experiments, and I'm starting to seriously warm up for option (2). Main reason: it would allow me to get rid of two classes: ComponentBase and PureComponentBase, which are workarounds for some JSInterop limitations. Thus, while we'll introduce 7 lifecycle interfaces, the only two ES6 classes which will be introduced to the codebase would be Component and PureComponent. Very neat.

... and if someone would still want to use ReactClassSpec, he can now mix it up with the component lifecycle interfaces too, for extra type safety.

[ivmarkov]

Done now - I think much simpler and nicer.

[pstockley]

That seems much cleaner. I will try and take a look early next week.

[pstockley]

I have taken a look at the changes. I am going to take them and make a number of additional changes. Specifically:

1) I think having a private constructor on Component will result in some strange bugs for JsInterop beginners. If your forget to add JsConstructor to your subclass you will get undefined errors. So I think it is better to keep it public. You can always make it private in your base class if you really care. I think the chances of someone accidentally constructing a component are rare.

2) I made props a property like you did for state. This is more in line with the JS implementation and I think it reads better than a getter.

3) The biggest issue I had was the way you were obtaining the constructor function using eval. From my testing, eval is very bad for performance (more than 10x slower in Chrome) and also not really safe. I added an additional helper to the JsHelper class that avoids eval. We won't then need JsVarargsFunction.

I have a number of other general changes I want to land. I am away next week so it will probably be the week after before I commit them. I really appreciate the work you did to prove out the concept. I will be deprecating the old createClass method and then removing it after a couple of months.

[ivmarkov]

1. I think having a private constructor on Component will result in some strange bugs for JsInterop beginners. If your forget to add JsConstructor to your subclass you will get undefined errors. So I think it is better to keep it public. You can always make it private in your base class if you really care. I think the chances of someone accidentally constructing a component are rare.

The Component class in my pull request has not private, but protected constructor. (Private constructor won't work in Java, because you then cannot inherit from the Component class, as there is no visible constructor to call with super(...).) I don't think there will be any issues with the protected constructor and JSInterop, as long as the constructor is annotated with "@JsConstructor", as it should anyway, but if you see such issues - change to public.

2. I made props a property like you did for state. This is more in line with the JS implementation and I think it reads better than a getter.

... on the other hand, the user might override it by using this.props = my_new_props which might result in an undefined behavior. Note that using this.state = my_new_state is allowed, in the Component constructor. Anyway, I don't have strong feelings for either approach, as both have its pros and cons.

3. The biggest issue I had was the way you were obtaining the constructor function using eval. From my testing, eval is very bad for performance (more than 10x slower in Chrome) and also not really safe. I added an additional helper to the JsHelper class that avoids eval. We won't then need JsVarargsFunction.

I would be very interested to see your solution. I can easily see how eval() can be removed, however I have some concerns with the removal of JsVarargsFunction. If you remove it, how do you model the case where the user has decided to export a @JsType annotated component in JavaScript under a name different from the Java Class<> name?

[pstockley]

I have added a similar but more react specific approach to handling component constructor functions. It does support passing both a Class and fully qualified class name in as a string. In our project we strip class meta data so we will need to use the qualified string name.

I am doing some further testing. Now we have switched to ES6 classes I want to make it work with Preact in addition to React.

[pstockley]

I taken all your changes and incorporated them with mine. Let me know if you have any issues. If not, I will close this pull request.

[ivmarkov]

Overall I think the changes that landed in your master branch are great! I have a small issue and a suggestion regarding ComponentConstructorFn, but more on that later.

Two small questions, minor priority:

• You have removed all pre-ES6-style component creation code. Did you port all your codebase to the new code?
• Is the new GWTReact codebase now compatible with preact? (Not that this is a priority for me, just asking because you mentioned it once.)

Regarding ComponentConstructorFn:

1. What I like is that there is no more eval(). Your namespacing iterator code is simple and straightforward and should probably end up in gwt-interop-utils.
2. What is a bit strange is the ComponentConstructorFn.onCreate(P props) method. It is not called anywhere, and I suspect it is just a stub so that GWT does not complain that you have annotated an empty interface with @JsFunction. But in that case probably a method with the following signature:

   interface ComponentConstructorFn<T>
   T create(Object... args);
   }

   is more appropriate, because it will hint at the fact, that the function has a constructor nature. Following additional advantages of this approach:

   • Your methods React.createElement(...) that take ComponentConstructorFn are not really type safe. That's because you have an unreferenced T generic parameter in the return type of these methods, which is not present in the input parameters of the method. Having ComponentConstructorFn<T> instead of the useless "P" generic parameter would fix that.
   • Method ComponentConstructorFn<P> getCtorFn(Class<T> cls) will become ComponentConstructorFn<T> getCtorFn(Class<T> cls) which is also more type safe.
   • Last but not least: if you additionally rename ComponentConstructorFn to just JsConstructorFn and ComponentUtils to JsConstructorUtils (let's face it: the relation of these classes to React is not material anyway), you can move both of these to gwt-interop-utils. Coupled with a new static utility method, <T> T JsConstructorUtils.create(JsConstructorFn<T>) method implemented in JavaScript that just calls 'new' on the passed constructor function, and suddenly you have a generic way to create and call JavaScript constructors.

That was the idea of my JSVarargsFunction as well. It is only that I followed the Java 8 functions package in spirit and named it after its signature, rather than after what it might be most useful for - dealing with JS constructor functions.

[pstockley]

Two small questions, minor priority:

You have removed all pre-ES6-style component creation code. Did you port all your codebase to the new code? (We are just starting to use the library in production so I thought it was better to remove the old code now given that React is going to deprecate it eventually. This also opens up the opportunity to use Preact).

Is the new GWTReact codebase now compatible with preact? (Not that this is a priority for me, just asking because you mentioned it once.) (Yes the library is compatible with preact with the additional React compatibility library. We are actually going to use Preact rather than React in our application.)

Regarding ComponentConstructorFn:

There are a couple of use cases for having a more React specific constructor function. Firstly, they give you a way of referencing React components in external JS libraries. For example, React Router has:

@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "ReactRouter")
public class ReactRouter {
    public static ComponentConstructorFn<RouterProps> Router;
    public static ComponentConstructorFn<RouteProps> Route;
    public static ComponentConstructorFn<RouteProps> IndexRoute;
    public static ComponentConstructorFn<RedirectProps> Redirect;
    public static ComponentConstructorFn<RedirectProps> IndexRedirect;
}

@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object")
public class RouteProps extends BaseProps {
    @JsProperty
    native <P extends BaseProps> void setComponent(ComponentConstructorFn<P> c);

    @JsProperty
    native <P extends BaseProps> void setComponents(StringMap<ComponentConstructorFn<P>> c);
}

The props parameter gives you a clue what type of props the component takes and also helps with auto completion when using React.createElement(...). For example, if you do

React.createElement(ComponentUtils.getCtorFn(StatefulExample.class), statefulCompProps),

The type of statefulCompProps needs to match the type of props defined by the StatefulExample component. If you use the getCtorFn method with just a string parameter you don't get this check unfortunately. I was thinking of changing this method to pass the class just for type checking purposes e.g.

public static <P extends BaseProps, S extends JsPlainObj, T extends Component<P, S>> ComponentConstructorFn<P> getCtorFn(Class<T> cls, String actualClassName) {.}

If you have any better ideas that still allow correct type checking of props I am open to suggestions.

I do like the idea of changing the method on ComponentConstructorFn from onCreate to create as it makes it more clear the purpose.

[ivmarkov]

OK. Type safety on the constructor input parameters makes it more complex, and it is probably fine then to give up on trying to make ComponentConstructorFn more generic. Yet, something like this is probably a better compromise then:

/**
 * A constructor function for a React Component
 */
@JsFunction
public interface ComponentConstructorFn<P extends BaseProps, T extends Component<P, ? extends JsPlainObj>> {
    /**
     * This method is not supposed to be called by the user directly, as it will not have the expected effect.
     * Calling a JavaScript function with 'new' is not supported from Java.
     */
    T create(P props);
}

To illustrate my point, I rebased my pull request on top of your changes - perhaps you can take a look. While I haven't changed the projects depending gwt-react yet, the above suggestion leads to minimal changes in React.java and ComponentUtils.java, so it is probably not that bad.

[pstockley]

I think what you have is a great solution for Components defined in Java. However, it doesn't work for components in Javascript libraries. In this case you don't want to define a Component class, you just need a reference to the constructor that is defined in Javascript. That is why I ended at the compromise I did.

[ivmarkov]

OK, point taken. Let's leave it as it is. If I come up with other ideas once I use it a bit, I'll let you know.