Integrate VDOM system into widget-core #728
Conversation
…l projector nodes
… output onto existing HTML
src/vdom.ts
Outdated
| filterAndDecorateChildren(child.children, instance); | ||
| } | ||
| } | ||
| } else { |
…meout when requestIdleCallback isn't supported
src/WidgetBase.ts
Outdated
| @@ -156,6 +124,20 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E | |||
| this.own(this._nodeHandler); | |||
| this._boundRenderFunc = this.render.bind(this); | |||
| this._boundInvalidate = this.invalidate.bind(this); | |||
| this.own(this.on('element-created', ({ key, element }: any) => { | |||
There was a problem hiding this comment.
you can use a map here, which returns a single composite handle:
this.on({
'element-create'({ key, element }: any) { /* ... */ },
'eleement-updated'({ key, element: any) { /* ... */ },
...
});
Also it seems like the any could/should be more specific?
There was a problem hiding this comment.
Nice I'll give that a try, also add the proper typings over any.
| } | ||
|
|
||
| protected invalidate(): void { | ||
| public invalidate(): void { |
There was a problem hiding this comment.
It now get's called from outside of the widget instance (in vdom), previously it was called from within the instance in dNodeToVNode
src/d.ts
Outdated
| if (typeof classes === 'function') { | ||
| classes = classes(); | ||
| properties = assign(properties, { classes }); | ||
| if (properties) { |
There was a problem hiding this comment.
Not sure I can see how properties could be undefined here?
There was a problem hiding this comment.
Yup, I think could possibly remove that (it's because at one point I allowed undefined properties and changed my mind) - but there is a change following these that will remove this entire section so perhaps wait for that. I don't mind.
| import { v } from './../d'; | ||
| import { VNode } from '@dojo/interfaces/vdom'; | ||
| import { InternalHNode } from './../vdom'; |
There was a problem hiding this comment.
We probably need to get rid of VNode in interfaces after we land this, right? It is used in a couple places (like test-extras) which sort of had to deal with maquette nodes directly.
There was a problem hiding this comment.
Yup we can remove them afterwards for sure and yup test-extras is going to suffer the most from these changes. Otherwise updating widgets was just a matter of swapping VNodes for DNodes
| @@ -29,6 +29,12 @@ global.requestAnimationFrame = (cb: (...args: any[]) => {}) => { | |||
| return true; | |||
| }; | |||
|
|
|||
| global.requestIdleCallback = (cb: (...args: any[]) => {}) => { | |||
There was a problem hiding this comment.
reminder that we would want to address this in https://github.com/dojo/test-extras/blob/master/src/support/loadJsdom.ts as well
| @@ -54,7 +46,8 @@ registerSuite({ | |||
| const widget = new TestWidget(); | |||
| widget.append(div); | |||
|
|
|||
| resolveRAF(); | |||
| resolvers.resolve(); | |||
There was a problem hiding this comment.
why are you calling this twice here and only once elsewhere? It was only called once before.
There was a problem hiding this comment.
@tomdye this is because after the render we need to resolve the requestIdleCallbacks, which with meta can cause an invalidation. So means we need to resolve the requestAnimationFrame (and idle callbacks again) to reflect the outcome from the render.
|
@agubler there is a regression from maquettes behaviour. Previously |
|
@kitsonk that’s correct, removing the support for ids and classes to be able to be passed as part of the selector was a conceous decision (the code logic was complicated and is pretty much considered an anti-pattern within Dojo 2). It’s one of the few areas where the behaviour between the two implementations differ (on purpose!). |
|
@agubler is it worth throwing then on strings that contain anything but |
|
@kitsonk I'm not sure there would be too much value, it would only prevent using If it becomes an issue with consumers we can possibly look to address it later? |
Well, it wasn't supported because those were broken out and applied to the DOM node.
Yeah, fine. |
Type: feature
The following has been addressed in the PR:
Description:
Integrates a fork of Maquette into widget-core that has been converted to deal with the widget-core
DNodeabstraction. All widget instance management has been moved fromWidgetBaseinto thevdomfunctionality.Achieves at least feature and performance parity with the existing implementation.
Includes updated variable names and code styling but maintains large amounts of the existing architecture and logic from Maquette which will be improved on over time.
Performance Benchmark Comparison between 0.1.2 (current) and 0.2.0 (new vdom)
Using js-framework-benchmark
Resolves #725