GWTReact / gwt-react

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

Re-introduce GwtReact components based on React's ES6 & other small cleanups #3

Closed ivmarkov closed 7 years ago

ivmarkov commented 7 years ago

Hey Paul,

(NOTE: The pull request is just "food for thought", and I don't think you should merge it right now.)

With the above said, the crux of the changes is around the following idea about stateful components - rather than doing: public static ReactClass<TodoListProps> component = React.createClass(new TodoList()); ... and then: return React.createElement(TodoItem.component, todoProps);

.. which looks quite strange in the Java world where there is quite a difference between an object, and its class instance..

how about simply: return React.createElement(TodoItem.class, todoProps);

The above would work, provided that the @JsType name does match the Java class name, which is always a good idea IMO.

By the way, if the @JsType name and the Java class name do not match, you can still do: return React.createElement(JsTypeConstructor.get(TodoItem.JS_TYPE_NAME), todoProps);

As part of the above changes, I also rebased GWTReact on top of the ES6 React components, as it looks a bit cleaner this way, and the new React tutorials for stateful components anyway start with the ES6 syntax.

I've also cleaned up a few other things like not using Java raw types and wrong native types signatures. Here's the full changeset description:

  1. Re-introduce GwtReact components based on React's ES6 Component/PureComponent.
  2. Simplify the signature of React.createElement for stateful components.
  3. Fix all places where raw types were used
  4. ReactDOM/ReactDOMServer: remove some unnecessary signatures (a DOMElement signature is not necessary if a ReactElement signature already exists; I don't think stateless functions allowed to be passed directly like that)
pstockley commented 7 years ago

Thanks, I was planning on looking at supporting ES6 classes again at some point. Originally I went down that path but GWT compiler problems in the earlier beta's gave me problems. It will probably take a couple of weeks before I get a chance to go over the changes in detail.

I am curious how you handle binding "this" on methods you need access to react element methods like set setState ? The React.createClass method takes care of that for you.

ivmarkov commented 7 years ago

Thanks for the quick feedback - no hurry at all for me - I'm just researching/evaluating on possible strategies for our future GWT/"modern-js-lib-solution" hybrid, just like you did.

As for the "this" problem, if you mean: https://medium.com/@housecor/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56#.egj7dj6aa

.. surprisingly to me, the generated GWT code seems to handle this automatically. In other words, if the render() method of my component returns something like this: return div(null, React.DOM.span(new HtmlProps().onClick(this::handleClick), "hi!")); ... then in the handleClick() method of my component, I have the proper, Java "this", bound to the component instance. I.e. this: Window.alert(this.getClass());

outputs the Java name of my component. Moreover, the "this" is already preset correctly in the render() method of the component and so on.

I don't have a firm explanation of the behavior (yet). It could be, because GWT always generates ES5 JS syntax even when inheriting from ES6 classes, or because there are quite a few autogenerated calls to apply() when GWT spits out its Java lambda machinery emulation code, or both.

ivmarkov commented 7 years ago

BTW as I hinted already, I value the following little gem even more than the ES6 components' rebase itself:

Creation of an element for a component, new way: return React.createElement(TodoItem.class, todoProps);

vs the old way: Step 1: public static ReactClass<TodoListProps> component = React.createClass(new TodoList()); <-- that looks very strange in Java Step 2: return React.createElement(TodoItem.component, todoProps);

The new approach would probably feel more natural to Java programmers.

pstockley commented 7 years ago

The binding issue is going to be a problem if you try and call a react method on the element. Try creating a component with some state and then calling the setState method within one of your methods.

A good example of what I am talking about is shown on the React Website here; https://facebook.github.io/react/docs/handling-events.html

ivmarkov commented 7 years ago

If you mean calling a React method from within e.g. the event callback handler, it works. See attached a simple component example that works for me: MyComp.zip

Note also that I just updated the pull request with exposing the "state" property exactly the way it is done in the React component samples following the ES6 approach: i.e. with a protected "state" field in the base component class. That's necessary so as the initial state to be initialized in the component constructor by just saying "this.state = your initial state" without using the setState() method, which does shallow merging, not initialization.

An alternative to that would be to expose in Java the "state" property via a getState()/setState() pair of property accessors and rename the merging setState() method in Java to i.e. updateState() with a @JSMethod(name = "setState") annotation. But I don't think this is cleaner than the above approach.

pstockley commented 7 years ago

That's good if it works. Not quite sure how it works at the moment. The GWT compiler must be binding the functions in some way. Let me have a play around with your changes and I will get back to you.

ivmarkov commented 7 years ago

Well the GWT compiler of course must emulate the Java "this" semantics, so I wouldn't be surprised if they preserve it at the jsinterop call sites as well. Easiest is to ask Goktug on the GWT Contrib forum.

pstockley commented 7 years ago

Hopefully, this week I will get some time to look at your Component changes. I would like to break this into some more smaller sets of changes. Could you create a pull request just with the raw type changes and fixes to the comments.

We can then do the other changes in a separate set of changes.

ivmarkov commented 7 years ago

No prob, but it might take a few days (something urgent popped up here). I'll ping you once I have it.

ivmarkov commented 7 years ago

See #5

ivmarkov commented 7 years ago

If you don't mind, I'll open a new pull request for the ES6 Component changes. In the meantime I've reworked them to have a much smaller change surface, and to preserve full backwards compatibility with the "classic api" of yours.

pstockley commented 7 years ago

Thanks. That was what I was going to suggest. That way I can phase out the old approach.