React migration #1914

jbicker · 2018-05-16T17:05:29Z

Proof of concept with StatusBar and ExtensionManager

akosyakov · 2018-05-17T03:15:53Z

packages/core/src/browser/widgets/react-widget.ts

+                this.childContainer = this.node;
+            }
+        }
+        ReactDOM.render(child, this.childContainer);


https://reactjs.org/docs/react-dom.html#unmountcomponentatnode - it should be called on dispose at least, otherwise react components will leak

akosyakov · 2018-05-17T03:24:05Z

packages/core/src/browser/status-bar/status-bar.tsx

@@ -0,0 +1,192 @@
+/*


Could you remove status-bar.old.ts that we are capable to see real changes in this file?

akosyakov · 2018-05-17T03:36:10Z

packages/core/src/browser/widgets/react-widget.ts

+        this.onRender.dispose();
+    }
+
+    protected render(): ReactElement<any> {


How about making ReactWidget abstract and replace it with abstract protected render(): MaybeArray<ReactElement<any>>;

akosyakov · 2018-05-17T03:37:13Z

packages/core/src/browser/widgets/react-widget.ts

+            }
+        }
+        ReactDOM.render(child, this.childContainer);
+        this.onRender.dispose();


It should go to the callback:
ReactDOM.render(child, this.childContainer, () => this.onRender.dispose());

akosyakov · 2018-05-17T03:38:43Z

packages/core/src/browser/widgets/react-widget.ts

+import { ReactElement } from "react";
+
+@injectable()
+export class ReactWidget extends BaseWidget {


Please add a deprecation note to VirtualWidget and put that ReactWidget should be used instead.

akosyakov · 2018-05-17T03:43:04Z

packages/core/src/browser/status-bar/status-bar.tsx

+import * as React from "react";
+import { StatusBarElements } from './status-bar-elements';
+
+export interface StatusBarLayoutData {


please rebase, it seems to contain code which was removed on master

akosyakov · 2018-05-17T03:51:23Z

packages/core/src/browser/status-bar/status-bar.tsx

+        return <StatusBarView leftElements={leftElements} rightElements={rightElements} />;
+    }
+
+    protected createAttributes(entry: StatusBarEntry): StatusBarEntryAttributes {


it will produce a new function each time. It would be interesting to try to add https://github.com/palantir/tslint-react, it has useful rules as jsx-no-bind, jsx-no-lamba to catch such issues in jsx.

It seems that StatusBarEntryAttributes is superflous and createAttributes can be inlined to renderElement as jsx:

return <div className={entry.command ? 'element hasCommand' : 'element'} title={entry.tooltip} onClick={() => {...}} />

after that rules should be able to catch bogus onClick={() => {...}}

akosyakov · 2018-05-17T03:53:15Z

packages/core/src/browser/status-bar/status-bar.tsx

+        const leftElements: React.ReactElement<StatusBarElements> = <StatusBarElements key="statusbar-left-elements" alignment="left" entries={leftEntries} />;
+        const rightElements: React.ReactElement<StatusBarElements> = <StatusBarElements key="statusbar-right-elements" alignment="right" entries={rightEntries} />;
+
+        return <StatusBarView leftElements={leftElements} rightElements={rightElements} />;


we should allow an array, as well, a return type should be MaybePromise<ReactElement<any>> in the superclass, after that StatusBarView is superflous and can be inlined to [leftElements, rightElements]

akosyakov · 2018-05-17T03:56:57Z

packages/core/src/browser/status-bar/status-bar.tsx

+                rightEntries.push(this.renderElement(entry));
+            }
+        });
+        const leftElements: React.ReactElement<StatusBarElements> = <StatusBarElements key="statusbar-left-elements" alignment="left" entries={leftEntries} />;


StatusBarElements and StatusBarView are superfluous:

return [ <div key="statusbar-left-elements" className="area left">{leftEntries}</div>, <div key="statusbar-right-elements" className="area right">{rightEntries}</div> ];

akosyakov · 2018-05-17T03:59:33Z

packages/core/src/browser/status-bar/status-bar-elements.tsx

+
+export class StatusBarElements extends React.Component<StatusBarElements.Props> {
+    render() {
+        return <div key={"statusbar-elements-" + this.props.alignment + "-area"} className={"area " + this.props.alignment}>{this.props.entries}</div>;


fyi: keys are only making sense into an array, here you have the following structure:
<StatusBarElements><div key="..." /></StatusBarElements> - key is redundant as you can see

Generally when you use jsx keys are not necessary, they are needed only then jsx elements are collected into an array and only for direct members of the array

akosyakov · 2018-05-24T05:21:42Z

packages/core/src/browser/status-bar/status-bar.tsx

    }

    protected createAttributes(entry: StatusBarEntry): StatusBarEntryAttributes {
        const attrs: StatusBarEntryAttributes = {};

        if (entry.command) {
-            attrs.onclick = () => {
+            attrs.onClick = () => {


we should get rid of it: https://stackoverflow.com/questions/36677733/why-shouldnt-jsx-props-use-arrow-functions-or-bind

akosyakov · 2018-05-24T05:22:30Z

packages/core/src/browser/status-bar/status-bar.tsx

-        const leftElements = h.div({ className: 'area left' }, VirtualRenderer.flatten(leftEntries));
-        const rightElements = h.div({ className: 'area right' }, VirtualRenderer.flatten(rightEntries));
-        return VirtualRenderer.flatten([leftElements, rightElements]);
+        const leftElements = <div className="area left"><React.Fragment>{leftEntries}</React.Fragment></div>;


React.Fragment is redundant here, leftEntries already wrapped into div, the same for rightEntries

Without wrapping React.Fragment I get the usual warning:

Warning: Each child in an array or iterator should have a unique "key" prop. Check the top-level render call using <div>. See https://fb.me/react-warning-keys for more information. in div

Btw. React.Fragment does not replace div. Actually it does not render a html tag at all. As far as I understood one can use it to group a list of elements without setting key attribute.

i am not sure React.Fragment is proper fix. https://fb.me/react-warning-keys does not tell anything about it. They say there that each child of leftEntries and rightEntries should have a key. Are you sure that React.Fragment does the same?

As far as I understood the following blog post, yes:
https://reactjs.org/blog/2017/11/28/react-v16.2.0-fragment-support.html

cool, thanks for checking!

svenefftinge · 2018-05-31T08:17:03Z

packages/core/src/browser/widgets/react-widget.tsx

+        super.onUpdateRequest(msg);
+        const child = this.render();
+        if (!this.childContainer) {
+            // if we are adding scrolling, we need to wrap the contents in its own div, to not conflict with the virtual dom algo.


Is this still true for React?

svenefftinge · 2018-05-31T08:40:44Z

The statusbar is explicitly calling Widget.update() whenever the state changes.
It works, but I don't think it is the how it should be done with React. The state of a component should rather be registered as React.state such that it automatically renders, when there are changes.

That said, I don't know if there are any bad implications with the current approach has, given that it seems to be a smoother path for our migration (we already explicitly call update everywhere).

akosyakov · 2018-06-01T08:22:15Z

packages/core/src/browser/status-bar/status-bar.tsx

+    }
+
+    protected onclick(entry: StatusBarEntry): () => void {
+        return () => {


It creates a new function each time. We should use exactly the same function for all entries. It can be accomplished by retrieving an entry id from an event target.

svenefftinge · 2018-06-20T04:26:08Z

packages/extension-manager/src/browser/extension-widget.ts

        if (this.ready) {
            return [this.renderSearchField(), this.renderExtensionList()];
        } else {
-            const spinner = h.div({ className: 'fa fa-spinner fa-pulse fa-3x fa-fw' }, '');
-            return h.div({ className: 'spinnerContainer' }, spinner);
+            const spinner = this.creator.div({ className: 'fa fa-spinner fa-pulse fa-3x fa-fw' }, '');


I like how you managed to make it a minimal change.
But given that people will take our code as an example, we should at least use TSX.

svenefftinge · 2018-06-20T04:27:51Z

packages/core/src/browser/status-bar/status-bar.tsx

-    protected render(): h.Child {
-        const leftEntries: h.Child[] = [];
-        const rightEntries: h.Child[] = [];
+    getLayoutData(): StatusBarLayoutData {


I think you reintroduced these methods by accident?

svenefftinge · 2018-06-20T13:07:05Z

packages/core/src/browser/status-bar/status-bar.tsx

+import { ReactWidget } from '../widgets/react-widget';
+import * as React from "react";
+
+export interface StatusBarLayoutData {


not used, please remove

svenefftinge · 2018-06-20T13:07:10Z

packages/core/src/browser/status-bar/status-bar.tsx

+    backgroundColor?: string
+}
+
+export interface StatusBarEntryData {


svenefftinge · 2018-06-20T13:15:28Z

packages/core/src/browser/widgets/react-widget.tsx

+    protected onUpdateRequest(msg: Message): void {
+        super.onUpdateRequest(msg);
+        if (!this.childContainer) {
+            // if we are adding scrolling, we need to wrap the contents in its own div, to not conflict with the virtual dom algo.


Is this still the case with react?

Signed-off-by: Jan Bicker <jan.bicker@typefox.io>

svenefftinge

LGTM

jbicker requested review from svenefftinge and akosyakov May 16, 2018 17:05

jbicker force-pushed the react-migration branch from 1525fa4 to f2d84aa Compare May 16, 2018 17:07

svenefftinge changed the title ~~React migration~~ [Statusbar] React migration May 16, 2018

akosyakov reviewed May 17, 2018

View reviewed changes

jbicker force-pushed the react-migration branch from f42715f to aaf7881 Compare May 23, 2018 14:31

akosyakov reviewed May 24, 2018

View reviewed changes

jbicker force-pushed the react-migration branch 2 times, most recently from f4d82e9 to fbf205e Compare May 30, 2018 12:35

svenefftinge reviewed May 31, 2018

View reviewed changes

svenefftinge mentioned this pull request Jun 1, 2018

Creating React components with JSX instead of plain javascript #2008

Closed

akosyakov reviewed Jun 1, 2018

View reviewed changes

jbicker force-pushed the react-migration branch from fbf205e to 0ce03cf Compare June 12, 2018 16:43

jbicker changed the title ~~[Statusbar] React migration~~ [WIP] React migration Jun 12, 2018

jbicker force-pushed the react-migration branch 6 times, most recently from b0e9a53 to 3f4bbf8 Compare June 19, 2018 09:49

svenefftinge reviewed Jun 20, 2018

View reviewed changes

jbicker force-pushed the react-migration branch 3 times, most recently from 4c0eb95 to c509f5d Compare June 20, 2018 13:12

svenefftinge reviewed Jun 20, 2018

View reviewed changes

react migration

addb41c

Signed-off-by: Jan Bicker <jan.bicker@typefox.io>

jbicker force-pushed the react-migration branch from c509f5d to addb41c Compare June 20, 2018 13:26

jbicker changed the title ~~[WIP] React migration~~ React migration Jun 20, 2018

svenefftinge approved these changes Jun 20, 2018

View reviewed changes

jbicker merged commit 7da249d into master Jun 20, 2018

jbicker deleted the react-migration branch June 21, 2018 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React migration #1914

React migration #1914

jbicker commented May 16, 2018 •

edited

Loading

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 17, 2018

akosyakov May 24, 2018

akosyakov May 24, 2018

jbicker May 30, 2018

jbicker May 30, 2018

akosyakov May 30, 2018

jbicker May 30, 2018 •

edited

Loading

akosyakov Jun 1, 2018

svenefftinge May 31, 2018

svenefftinge commented May 31, 2018

akosyakov Jun 1, 2018

svenefftinge Jun 20, 2018

svenefftinge Jun 20, 2018

svenefftinge Jun 20, 2018

svenefftinge Jun 20, 2018

svenefftinge Jun 20, 2018

svenefftinge left a comment

React migration #1914

React migration #1914

Conversation

jbicker commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbicker May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge left a comment

Choose a reason for hiding this comment

jbicker commented May 16, 2018 •

edited

Loading

jbicker May 30, 2018 •

edited

Loading