Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtual DOM and JSX support #9967

Closed
wants to merge 21 commits into from
Closed

Virtual DOM and JSX support #9967

wants to merge 21 commits into from

Conversation

johnspackman
Copy link
Member

This PR is about improving the Virtual DOM in qx.html.* and adding JSX support, primarily for use in server applications but also the browser.

The changes are:

  • the qx.html.Element is split into qx.html.Node and qx.html.Element, and also the addition of qx.html.Text. This change is entirely backwards compatible, and the addition of Text allows any arbitrary DOM structure to be represented (code in qx.ui.* does not have to use Text nodes because the UI classes always operate directly on the node to set/get text).
  • Add JSX Factory implementation
  • VDOM can be serialized into HTML text stream for delivery to the client browser
  • An entire tree of the DOM on the browser (ie that which was built because the browser parsed the HTML text) can be reattached to a matching or sparse tree of qx.html.Element instances, based on the qxObjectId
  • There are some additional properties and methods (eg cssClass, addCssClass(), visible, etc) which exist to simplify common tasks
  • Document the new qx.headless property in the compiler which indicates that the code is compiled for the server
  • There are a few warning fixes and other minor tweaks which have crept in

…ghtweight server apps which manipulate Virtual DOM rendered on the server
Copy link
Contributor

@cboulanger cboulanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very interesting and useful!

docs/desktop/gui/html.md Outdated Show resolved Hide resolved
docs/desktop/gui/html.md Show resolved Hide resolved
framework/source/class/qx/core/BaseInit.js Show resolved Hide resolved
this.error("'mousewheel' event took " + diff + "ms - this may have a significant effect on UI experience");
} else if (diff > 100) {
this.warn("'mousewheel' event took " + diff + "ms - consider refactoring for smoother UI experience");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that code got pulled in through rebase :(

Copy link
Member

@derrell derrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tagged this for a weekend review, as the changes are extensive. This is a very, very scary PR, though. It looks like it's mucking with low-level stuff that can affect all desktop apps. I'll spend time on it this weekend, but I suspect this would be best left until after the release of 6.0 because of the possibility of subtle breakage. That way, we'll have lots of time to test with it and not take the chance on messing up our first release in years.

framework/source/class/qx/bom/element/Attribute.js Outdated Show resolved Hide resolved
@johnspackman
Copy link
Member Author

@derrell I've tagged this for a weekend review, as the changes are extensive. This is a very, very scary PR, though. It looks like it's mucking with low-level stuff that can affect all desktop apps. I'll spend time on it this weekend, but I suspect this would be best left until after the release of 6.0

That's a fair point, and I was a bit torn myself. It has only had limited testing.

I'm happy to create a v7.x branch and make the PR in there?

Copy link
Member

@derrell derrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. There's a lot here! I spent about an hour on this, but could spend many more for a thorough review. Since this is supposed to be BC, I suggest we consider this for a 6.1 or 7.0 version, where it will get far more BC testing in existing apps before being officially released.

docs/desktop/gui/html.md Outdated Show resolved Hide resolved
@@ -353,7 +353,7 @@ qx.Bootstrap.define("qx.bom.client.Html",
* @ignore(Image)
*/
getDataUrl : function(callback) {
var data = new Image();
var data = new window.Image();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this change was needed? In the browser, Image is directly accessible via the global object window. In Nodejs, window doesn't exist unless qooxdoo has started up, in which case window and global are the same thing, but in that case, there is no Image global.Image...???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it suppresses the warnings because window is always an allowed/well known global but Image isn't when compiling for a NodeJS target. Part of my goal was to improve NodeJS development, by adding VDOM and reducing the unnecessary dependencies (because this can cause code to be loaded which crashes) - connected with this was removing warnings which are issued on nodejs-only targets.

In any code which has to be cross platform, rather than adding a blanket @ignore(window.*) instead I tend to use window.Image because it's direct about what the code is referencing

framework/source/class/qx/bom/element/Attribute.js Outdated Show resolved Hide resolved
framework/source/class/qx/event/handler/Application.js Outdated Show resolved Hide resolved
framework/source/class/qx/event/handler/Mouse.js Outdated Show resolved Hide resolved
framework/source/class/qx/html/Element.js Outdated Show resolved Hide resolved
framework/source/class/qx/html/Element.js Outdated Show resolved Hide resolved
framework/source/class/qx/html/Element.js Outdated Show resolved Hide resolved
hkollmann and others added 10 commits May 18, 2020 09:31
* bring tutorials to an more dominat place in doc
* fix docs
* Add warning about result of this.tr("")
* Add description of header file return on empty string to docu
* Fix unicode.org link

Thanks @zaucker !
…ghtweight server apps which manipulate Virtual DOM rendered on the server
Co-authored-by: Tobias Oetiker <tobi@oetiker.ch>
…rtual-dom

* 'virtual-dom' of github.com:johnspackman/qooxdoo:
  doc fix; add jsx code
  Update docs/desktop/gui/html.md
  lint & es5
  docs update
  improves support for server based Virtual DOM and adds support for lightweight server apps which manipulate Virtual DOM rendered on the server

# Conflicts:
#	framework/source/class/qx/bom/element/Attribute.js
#	framework/source/class/qx/html/Jsx.js
@johnspackman johnspackman added the do not merge DO NOT MERGE YET! label May 18, 2020
@johnspackman
Copy link
Member Author

I'm marking this as do not move for now, until we confirm what we're going to do for 6.1 release PRs

@hkollmann hkollmann marked this pull request as draft May 18, 2020 15:12
fixes a bug in events where DOM events and the `qx.html.Node` have the same event name
@johnspackman johnspackman deleted the virtual-dom branch February 4, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge DO NOT MERGE YET!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants