Switch to React from react-lite #9263

mareklibra · 2018-05-29T12:58:42Z

The react-lite is slowly becoming obsolete compared to the recent react version 16.4 and
there are recently no plans to add support of v16 features to the react-lite [1].

Fixes to various so far found issues are provided.

One of the first benefits of this change is alignment of Cockpit with requirements of paternfly-react library.

Some of the issues related to React 15 -> 16 upgrade can be found in [2] .

To use development version of React:

./autogen.sh --enable-debug
make

[1] Lucifier129/react-lite#126
[2] https://reactjs.org/blog/2017/09/26/react-v16.0.html

List of follow-ups:

remove commented out code in cockpit-components-listing.jsx
redesign show_modal_dialog
check-{machines|ovir} - provide infra for setting checkbox value
pkg/storage - refactor component hierarchy of LogsPanel and content-views.jsx:block_rows()
test/check-* - provide infra for setting form fields rendered by React - text input, checkbox, etc.

PR split - following will be removed from here and merged separately:

Manual UI click arounds

Issues found so far

Tooltip placement on Storage page (it's correct on e. g. Software Updates)
Machines: create: cannot toggle "immediately start VM" check box
Machines: switching to serial console crashes with TypeError: this.state.terminal.renderer is undefined (Oops happens with current master too, but at least the terminal appears)
Subscription register dialog is unresponsive, can't type into text fields.

mareklibra · 2018-05-29T13:42:06Z

Waiting for tests to see remainers.

So far verified on my local dev environment, farther testing is needed.

Please build with ./autogen.sh --enable-debug to use development version of React - i.e. propType checking and better error messages are available.

mareklibra · 2018-05-31T06:25:38Z

pkg/lib/cockpit-components-listing.jsx

        if (!this.props.children || this.props.children.length === 0) {
            headerClasses = "listing-ct-empty";
            headerRow = <tr><td>{this.props.emptyCaption}</td></tr>;
        } else if (this.props.columnTitles.length) {
+            /* probably not needed, ListingRow.selected prop should be set to 'false' by default


Keeping it here for simpler revert if I'm wrong.

Will be removed later if no issue found.

mareklibra · 2018-05-31T06:27:47Z

pkg/lib/cockpit-components-dialog.jsx

-        body: React.PropTypes.element.isRequired,
-        footer: React.PropTypes.element.isRequired,
-        id: React.PropTypes.string,
+        title: PropTypes.string, // required, but show_modal_dialog provides it after initial rendering; TODO: fix


I would prefer to fix the modal dialog in a follow-up.
Maybe by using patternfly-react's component for it but various Cockpit's use cases needs to be considered.

mareklibra · 2018-05-31T06:29:07Z

pkg/machines/components/consoles.jsx

-                    </Select.StatelessSelect>
-                </td>
-            </tr>
+            <tbody>


React is more strict about element nesting ...

mareklibra · 2018-05-31T06:30:51Z

test/verify/check-machines

@@ -408,7 +408,7 @@ class TestMachines(MachineCase):
        b.wait_present("#slate-header")
        b.wait_in_text("#slate-header", "Virtualization Service (libvirt) is Not Active")
        b.wait_present("#enable-libvirt:checked")
-        b.set_checked("#enable-libvirt", False)
+        b.click("#enable-libvirt") # uncheck it ; ; TODO: fix this, do not assume initial state of the checkbox


I prefer follow-up to fix this.

mareklibra · 2018-06-01T06:24:08Z

Reactifying continues. This time storaged.

mareklibra · 2018-06-05T09:24:17Z

Failing check-machines: testCreate on rhel-7 is fixed by #9304

mareklibra · 2018-06-06T06:37:58Z

#9304 is merged, restarting tests

martinpitt · 2018-06-08T07:22:15Z

@mareklibra : Thanks for working on this! One hint, if you want to land this faster: A lot, or even most, commits also apply to react-lite, so they could land in a separate PR. This gets easier to review and validate in several smaller steps, and then the actual switch to React can happen separately.

martinpitt · 2018-06-08T07:24:36Z

pkg/docker/containers-view.jsx

 var Listing = require('cockpit-components-listing.jsx');
 var Select = require('cockpit-components-select.jsx');
 var moment = require('moment');

-var Dropdown = React.createClass({
+var Dropdown = createReactClass({


This is already processed as ES6, so this could also just use the normal class .. extends React.Component syntax, couldn't it? Also, it could use import instead of var .. require? Or does that require even more intrusive changes? If so, then fine to do this intermediate step.

Since this PR is about to be split into smaller ones now, we can afford this kind of refactoring.
Originally I skipped that to ease review.

I hope I'm not wrong, but I expect just syntax changes with this replacement.

mareklibra · 2018-06-08T07:44:24Z

A lot, or even most, commits also apply to react-lite, so they could land in a separate PR.

This task became much bigger than originally expected. This will help.

React does this automatically in a high-performance way, so this call is not needed in this context.

PropTypes were moved to separate npm package 'prop-types' from core 'React'. This patch is part of "react-lite to React" series. So far, the PropTypes where just declared but not actually used. Bunch of inconsistencies are expected, fixes will follow in next patches.

The `React.createClass` has been removed from React. For legacy code, there's `create-react-class` npm package.

The react-lite library keeps compatibility with React 15.x which is slowly deprecating. Various Cockpit's npm dependencies are starting to require React 16 features, namely i.e. patternfly-react. Closes cockpit-project#9263

- More "key" properties - A <tbody> for each <table> - Don't bind methods

React wont allow it; setting the "value" attribute will not be reflected in the "value" property for arbitrary divs. So we make this more explicit by using a custom attribute.

Use Browser.set_input_text as appropriate.

martinpitt · 2018-09-19T08:59:53Z

Pushed a rebase to fix the current conflicts.

martinpitt · 2018-09-19T14:41:50Z

I'm at an impasse with the console crash. This is known issue #9641 again, it just crashes a lot harder now. @patternfly/react-console 1.4.2 contains the fix, but it also pulls in @novnc/novnc, and that causes an error:

SyntaxError: export declarations may only appear at top level of a module

here in the generated machines.js webpack:

/***/ function(module, exports) {

	/*
	 * noVNC: HTML5 VNC client
	 * Copyright (C) 2012 Joel Martin
	 * Licensed under MPL 2.0 (see LICENSE.txt)
	 *
	 * See README.md for usage and integration instructions.
	 */
	
	/*
	 * Logging/debug routines
	 */
	
	var _log_level = 'warn';
	
	var Debug = function (msg) {};
	var Info = function (msg) {};
	var Warn = function (msg) {};
	var Error = function (msg) {};
	
	export function init_logging (level) {

This fixes the pesky crash at last. This pulls in @novnc/novnc, which uses ES6 syntax in its *.js files. This leads to runtime errors like SyntaxError: export declarations may only appear at top level of a module as these are embedded in webpacks. Until we update to a less ancient weback, fix these by applying babel on the novnc sources.

martinpitt · 2018-09-19T15:19:08Z

I bent the new PF-reactconsole to my will, and it's behaving correctly now. Yay for that crash being gone at last!

- Added "key" properties and "tbody" elements as necessary - "Listing" wants an array of actions - The "event" structure given to a handler is only valid while the handler runs

mvollmer · 2018-09-20T09:06:25Z

Tooltip placement is off.

I can not reproduce this anymore. :-/

- "key" properties and "tbody" elements as necessary - Form elements must be re-rendered on every change - use StatelessSelect as required by the re-rendering - Use Browser.set_input_text instead of set_val

- Use strings for "key" properties. - Construct a new dialog body when values change, a render() call is not enough. - Plumb "startVm" thtough the state as well to ensure re-rendering. - Be sure to set text input values always to a string (and not "undefined") so that React will always treat them as controlled components. - Use "className" instead of "extraClass", which is not a thing. The additional class doesn't seem to have any effect, though.

- Pass a empty values for required properties instead of "undefined".

martinpitt · 2018-09-23T07:07:07Z

Whee, all green and all TODOs done 🎉 Nice job everyone!

The react-lite library keeps compatibility with React 15.x which is slowly deprecating. Various Cockpit's npm dependencies are starting to require React 16 features, namely i.e. patternfly-react. Closes #9263

The react-lite library keeps compatibility with React 15.x which is slowly deprecating. Various Cockpit's npm dependencies are starting to require React 16 features, namely i.e. patternfly-react. Closes cockpit-project#9263

mareklibra mentioned this pull request May 29, 2018

feat(VncConsole): Introduce VncConsole component patternfly/patternfly-react#288

Merged

mareklibra changed the title ~~Switch to React from react-lite~~ WIP: Switch to React from react-lite May 29, 2018

mareklibra force-pushed the react branch from 2bfc442 to c4d9533 Compare May 31, 2018 06:22

mareklibra commented May 31, 2018

View reviewed changes

mareklibra force-pushed the react branch 2 times, most recently from 0cb84e4 to f3f2694 Compare June 1, 2018 06:15

mareklibra added the needswork label Jun 1, 2018

mareklibra force-pushed the react branch from feac559 to c6068e5 Compare June 4, 2018 09:29

mareklibra mentioned this pull request Jun 6, 2018

packagekit: Swap out many divs and spans for React.Fragment #9309

Merged

mareklibra force-pushed the react branch 2 times, most recently from aeb398c to 811f96a Compare June 7, 2018 08:57

martinpitt reviewed Jun 8, 2018

View reviewed changes

atiratree mentioned this pull request Jun 21, 2018

kubernetes: virtual machines - add Vm Detail page #9466

Closed

3 tasks

mareklibra and others added 8 commits September 19, 2018 10:25

cockpit-components-dialog: Remove unnecessary .bind() call

f3eaed2

React does this automatically in a high-performance way, so this call is not needed in this context.

Fix React's createClass reference

0e3c7e1

The `React.createClass` has been removed from React. For legacy code, there's `create-react-class` npm package.

Use React instead of react-lite

714f4aa

The react-lite library keeps compatibility with React 15.x which is slowly deprecating. Various Cockpit's npm dependencies are starting to require React 16 features, namely i.e. patternfly-react. Closes cockpit-project#9263

docker: Fixes for full React

981013f

- More "key" properties - A <tbody> for each <table> - Don't bind methods

storage: Don't abuse the "value" property to communicate with tests

286dba8

React wont allow it; setting the "value" attribute will not be reflected in the "value" property for arbitrary divs. So we make this more explicit by using a custom attribute.

storage: Clear JobsPanel timers on unmount

9e535ac

test: Fix check-machines and check-ovirt for React

d5101f3

Use Browser.set_input_text as appropriate.

martinpitt removed the needs-rebase label Sep 19, 2018

martinpitt force-pushed the react branch from 04b5d2c to d5101f3 Compare September 19, 2018 08:59

mvollmer added 2 commits September 20, 2018 11:23

networking: Adapt to full React

a9d9248

- Added "key" properties and "tbody" elements as necessary - "Listing" wants an array of actions - The "event" structure given to a handler is only valid while the handler runs

storage: More "key" props

986ca33

subscriptions: Adapt to full React

092a04c

- "key" properties and "tbody" elements as necessary - Form elements must be re-rendered on every change - use StatelessSelect as required by the re-rendering - Use Browser.set_input_text instead of set_val

mvollmer force-pushed the react branch 3 times, most recently from 0e8fcba to 3d3adde Compare September 21, 2018 10:50

mvollmer force-pushed the react branch from 3d3adde to 617eb5f Compare September 21, 2018 11:03

ovirt: Adapt to full React

0ad4e8c

- Pass a empty values for required properties instead of "undefined".

mvollmer force-pushed the react branch from ddfd9bb to 0ad4e8c Compare September 21, 2018 12:04

martinpitt removed the needswork label Sep 21, 2018

martinpitt merged commit 46612ec into cockpit-project:master Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to React from react-lite #9263

Switch to React from react-lite #9263

mareklibra commented May 29, 2018 •

edited by mvollmer

Loading

mareklibra commented May 29, 2018

mareklibra May 31, 2018

mareklibra May 31, 2018

mareklibra May 31, 2018

mareklibra May 31, 2018 •

edited

Loading

mareklibra commented Jun 1, 2018

mareklibra commented Jun 5, 2018

mareklibra commented Jun 6, 2018

martinpitt commented Jun 8, 2018

martinpitt Jun 8, 2018

mareklibra Jun 8, 2018 •

edited

Loading

mareklibra commented Jun 8, 2018

martinpitt commented Sep 19, 2018

martinpitt commented Sep 19, 2018

martinpitt commented Sep 19, 2018

mvollmer commented Sep 20, 2018

martinpitt commented Sep 23, 2018

Switch to React from react-lite #9263

Switch to React from react-lite #9263

Conversation

mareklibra commented May 29, 2018 • edited by mvollmer Loading

mareklibra commented May 29, 2018

mareklibra May 31, 2018

Choose a reason for hiding this comment

mareklibra May 31, 2018

Choose a reason for hiding this comment

mareklibra May 31, 2018

Choose a reason for hiding this comment

mareklibra May 31, 2018 • edited Loading

Choose a reason for hiding this comment

mareklibra commented Jun 1, 2018

mareklibra commented Jun 5, 2018

mareklibra commented Jun 6, 2018

martinpitt commented Jun 8, 2018

martinpitt Jun 8, 2018

Choose a reason for hiding this comment

mareklibra Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

mareklibra commented Jun 8, 2018

martinpitt commented Sep 19, 2018

martinpitt commented Sep 19, 2018

martinpitt commented Sep 19, 2018

mvollmer commented Sep 20, 2018

martinpitt commented Sep 23, 2018

mareklibra commented May 29, 2018 •

edited by mvollmer

Loading

mareklibra May 31, 2018 •

edited

Loading

mareklibra Jun 8, 2018 •

edited

Loading