Add evented Node handler to meta #666

tomdye · 2017-09-08T15:12:13Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

adds nodehandler to meta
tracks elements, widget roots and projector creation / updates
changes the way nodes are acquired within meta
fixes tests
events for element creation

Resolves #662

matt-gadd · 2017-09-08T15:43:09Z

src/WidgetBase.ts

@@ -199,14 +203,9 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 		this._diffPropertyFunctionMap = new Map<string, string>();
 		this._bindFunctionPropertyMap = new WeakMap<(...args: any[]) => any, { boundFunc: (...args: any[]) => any, scope: any }>();
 		this._registries = new RegistryHandler();
+		this._nodeHandler = new NodeHandler();


we should own this

agubler

a few comments

agubler · 2017-09-08T15:42:18Z

src/NodeHandler.ts

+	Widget = 'Widget'
+}
+
+export default class NodeHandler extends Evented {


Can we make this a named export and export default below (a pattern for most widget-core modules)

agubler · 2017-09-08T15:42:58Z

src/NodeHandler.ts

+import { VNodeProperties } from '@dojo/interfaces/vdom';
+import Map from '@dojo/shim/Map';
+
+export enum Type {


Can we have some documentation on these (and the rest of the module?), is type descriptive enough?

agubler · 2017-09-08T15:44:31Z

src/NodeHandler.ts

+	}
+
+	public addRoot(element: Element, properties: VNodeProperties) {
+		this.add(element, properties);


Do you need the key check for adding the root too?

can we add a failing test for this if it needs the guard 😄

way ahead of you 😉

agubler · 2017-09-08T15:44:47Z

src/WidgetBase.ts

 	private _boundRenderFunc: Render;

 	private _boundInvalidate: () => void;

 	private _defaultRegistry = new WidgetRegistry();

+	private _nodeHandler: NodeHandler;
+
+	private _projectorMountEvent: any;


is this really any?

agubler · 2017-09-08T15:45:04Z

src/WidgetBase.ts

@@ -199,14 +203,9 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 		this._diffPropertyFunctionMap = new Map<string, string>();
 		this._bindFunctionPropertyMap = new WeakMap<(...args: any[]) => any, { boundFunc: (...args: any[]) => any, scope: any }>();
 		this._registries = new RegistryHandler();
+		this._nodeHandler = new NodeHandler();


Need to own this

agubler · 2017-09-08T15:47:17Z

src/WidgetBase.ts

 		while (nodes.length) {
 			const node = nodes.pop();
 			if (isHNode(node) || isWNode(node)) {
 				node.properties = node.properties || {};
 				if (isHNode(node)) {
-					if (node.properties.key) {
+					if (rootNodes.indexOf(node) < 0 && node.properties.key) {


why not === -1?

looks a bit confusing like that to me

agubler · 2017-09-08T15:47:53Z

src/interfaces.d.ts

- */
-export interface WidgetMetaRequiredNodeCallback {
-	(node: HTMLElement): void;
+	nodeHandler: any;


type? might need to create an interface in here for NodeHandler

agubler · 2017-09-08T15:48:02Z

src/meta/Base.ts


 export class Base extends Destroyable implements WidgetMetaBase {
 	private _invalidate: () => void;
 	private _invalidating: number;
-	private _requiredNodes: Map<string, ([ WidgetMetaBase, WidgetMetaRequiredNodeCallback ])[]>;
-	protected nodes: Map<string, HTMLElement>;
+	protected nodeHandler: any;


Type please

agubler · 2017-09-08T15:48:49Z

src/mixins/Projector.ts

@@ -286,6 +288,24 @@ export function ProjectorMixin<P, T extends Constructor<WidgetBase<P>>>(Base: T)
 			return this._projection.domNode.outerHTML;
 		}

+		_afterRootCreateCallback(element: Element,


formatting a bit weird here (and the other functions)

agubler · 2017-09-08T15:49:16Z

tests/unit/meta/Dimensions.ts


 let rAF: any;
+const defaultDimensions = {


Don't we export these from dimensions?

no, i did add that earlier but removed again, makes sense

agubler · 2017-09-08T16:43:25Z

src/NodeHandler.ts

+		this.emit({ type: key });
+	}
+
+	public addRoot(element: HTMLElement, properties: VNodeProperties) {


I know this is a bit picky, but you could have a single function that does this, and three specific functions that pass their type. Like:

public add(element: HTMLElement, properties: VNodeProperties) { if (properties.key) { this.add(element, properties); } this.emit({ type: properties.key }); } public addRoot(element: HTMLElement, properties: VNodeProperties) { this._add(element, properties, Type.Widget); } public addProjector(element: HTMLElement, properties: VNodeProperties) { this._add(element, properties, Type.Projector); } private _add(element: HTMLElement, properties: VNodeProperties = {}, type: Type) { if (properties.key) { this.add(element, properties); } this.emit({ type }); }

fair. but doesn't quite work, as a root or a projector may have a key and thus needs to be emited via it's key as well as via it's type

Isn't that logically actually same?

disclosure, I updated the code :)

agubler · 2017-09-08T16:47:25Z

src/WidgetBase.ts

+		this.onElementUpdated(element, String(properties.key));
+	}
+
+	_addElementToNodeHandler(element: Element, projectionOptions: ProjectionOptions, properties: VNodeProperties) {


is this private? let's add the visibility modifier.

agubler · 2017-09-08T16:48:16Z

src/WidgetBase.ts

+	_addElementToNodeHandler(element: Element, projectionOptions: ProjectionOptions, properties: VNodeProperties) {
+		this._currentRootNode += 1;
+
+		if (!this._projectorAttachEvent) {


minor nit: I believe if (this._projectorAttachEvent === undefined) { is more performant and also personally I find it clearer.

agubler · 2017-09-08T16:49:46Z

src/WidgetBase.ts

+		this._currentRootNode += 1;
+
+		if (!this._projectorAttachEvent) {
+			this._projectorAttachEvent = projectionOptions.nodeEvent.on('attached',


we should own this._projectorAttachEvent also

agubler · 2017-09-08T16:50:47Z

src/WidgetBase.ts

@@ -433,6 +440,7 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 	}

 	public __render__(): VirtualDomNode | VirtualDomNode[] {
+		this._nodeHandler.clear();


is this the correct place for this? so it clears regardless of whether the widget was invalidated.... I don't think that this is the equivalent to when it was executed in the @beforeRender...

agubler · 2017-09-08T16:52:16Z

src/interfaces.d.ts

@@ -409,20 +409,21 @@ export interface WidgetMetaConstructor<T extends WidgetMetaBase> {
 	new (properties: WidgetMetaProperties): T;
 }

+export interface NodeHandler extends Evented {


we tend to have doc on exported interfaces (we also tend to suffix with Interface)

agubler · 2017-09-08T16:52:52Z

src/meta/Dimensions.ts

@@ -23,7 +25,7 @@ export interface DimensionResults {
 	scroll: TopLeft & Size;
 }

-const defaultDimensions = {
+export const defaultDimensions = {


thinking about this, doesn't this mean that something external could mess with the defaults?

matt-gadd · 2017-09-10T13:07:48Z

src/meta/Dimensions.ts

+	constructor(properties: WidgetMetaProperties) {
+		super(properties);
+
+		this.nodeHandler.on(NodeEventType.Projector, () => {


i think this is going to put us in a loop no?

matt-gadd · 2017-09-10T13:08:57Z

src/WidgetBase.ts

+		this._currentRootNode += 1;
+
+		if (this._projectorAttachEvent === undefined) {
+			this._projectorAttachEvent = projectionOptions.nodeEvent.on('attached',


this isn't really attached or mount (which it previously was), and more after dom update no?

agubler · 2017-09-11T08:46:22Z

src/WidgetBase.ts

@@ -436,6 +445,7 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 		this._renderState = WidgetRenderState.RENDER;
 		if (this._dirty === true || this._cachedVNode === undefined) {
 			this._dirty = false;
+			this._nodeHandler.clear();


I still think that this is in the wrong place, it will clear the map before running the renders and therefore always return back the default dimensions! This is were integration tests for using the meta with WidgetBase come in handy.

pottedmeat · 2017-09-11T16:47:25Z

src/NodeHandler.ts

+	Widget = 'Widget'
+}
+
+export class NodeHandler extends Evented implements NodeHandlerInterface {


There are two things I've been struggling with using meta that this could perhaps help with. The first is doing a reverse lookup and the second is knowing what the currently rendered keys are. I need the reverse lookup for intersection meta where I have a single observer that receives multiple events. I don't have a good use case for knowing all the keys, but I could see it being potentially helpful/related to whatever we'd have to do for a reverse lookup.

agubler

couple more nits

agubler · 2017-09-11T17:46:35Z

src/NodeHandler.ts

+		return this._nodeMap.has(key);
+	}
+
+	public add(element: HTMLElement, properties: VNodeProperties) {


nit: can you add return type of : void please?

In other places through the PR also?

Maybe it's okay because it is defined on the interface, I don't know what the standards are around this. Can't hurt to add them I guess.

agubler · 2017-09-11T17:49:36Z

src/meta/Base.ts

 			}
+
+			const handle = this.nodeHandler.on(key, () => {


maybe should own this

agubler · 2017-09-11T17:50:38Z

src/mixins/Projector.ts

@@ -286,6 +289,24 @@ export function ProjectorMixin<P, T extends Constructor<WidgetBase<P>>>(Base: T)
 			return this._projection.domNode.outerHTML;
 		}

+		protected afterRootCreateCallback(element: HTMLElement,


I think this formatting is really funky :) is it different from the formatting in widget base?

I think you're really funky

Formatting is the same as in widgetbase

it's not the same, it is like this in widget base

protected afterRootCreateCallback( element: HTMLElement, projectionOptions: ProjectionOptions, vnodeSelector: string, properties: VNodeProperties ): void { // ... stuff }

notice the first param is on the next line?

my apologies, have fixed

pottedmeat · 2017-09-11T18:02:19Z

src/meta/Base.ts

-			if (!callback) {
-				this.invalidate();
+		if (!node) {
+			if (this._requestedNodeKeys.has(key)) {


Won't this throw an error if it's called twice? And is the assumption that .has would always be used to check to see if a node exists and never .getNode? Throwing an error as a side-effect of a getter could be unexpected.

Good catch re the error @pottedmeat. I'll remove the error and only add to the requested keys if it does not already exist in there. Thanks

agubler · 2017-09-13T19:15:39Z

src/WidgetBase.ts

@@ -274,7 +266,7 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 		this.onElementUpdated(element, String(properties.key));
 	}

-	protected afterRootUpdateCallback(
+	private afterRootUpdateCallback(


needs the _ prefix

agubler · 2017-09-13T19:15:51Z

src/WidgetBase.ts

@@ -251,7 +243,7 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 		this.onElementCreated(element, String(properties.key));
 	}

-	protected afterRootCreateCallback(
+	private afterRootCreateCallback(


needs the _ prefix

agubler · 2017-09-13T19:16:19Z

src/WidgetBase.ts

-		currentRootNode += 1;
-		currentRootNodeMap.set(this, currentRootNode);
-		const isLastRootNode = (currentRootNode === numRootNodes);
+		this._currentRootNode += 1;


why not this._currentRootNode++

matt-gadd reviewed Sep 8, 2017

View reviewed changes

tomdye force-pushed the nodeHandler branch from c350aa5 to 2deeff5 Compare September 8, 2017 15:45

agubler suggested changes Sep 8, 2017

View reviewed changes

agubler reviewed Sep 8, 2017

View reviewed changes

matt-gadd reviewed Sep 10, 2017

View reviewed changes

agubler reviewed Sep 11, 2017

View reviewed changes

tomdye force-pushed the nodeHandler branch from 7c3db34 to 1e80603 Compare September 11, 2017 16:20

pottedmeat reviewed Sep 11, 2017

View reviewed changes

agubler reviewed Sep 11, 2017

View reviewed changes

pottedmeat reviewed Sep 11, 2017

View reviewed changes

tomdye added 13 commits September 12, 2017 13:22

th progress

838903a

dimensions tests

ed9bc3e

fixed tests

6f75f56

fixed interface comments

f61028d

fixed Matches

8aa52fe

comments

2bb5260

remove beforeRender

35ca787

pr comments

66b4d05

fix typings

2d4e39f

add nodeEvent typing

47b4f75

rename Type enum

0162789

rename Type enum

43ab419

pr comments, changed dimensions implementation

ea370b8

tomdye added 10 commits September 12, 2017 13:24

added getnode function

1142321

updated tests

076085d

adding integration test

4d31f7d

integration tests

5decea2

uncomment tests

0aa14cc

fix test after changed functionality

e88241b

remove cancel raf

fda32dd

pr nits

b9787b5

pr alignment

3e50f14

change to weakmaps

6207249

tomdye force-pushed the nodeHandler branch from f90c8c1 to 6207249 Compare September 12, 2017 12:24

agubler approved these changes Sep 12, 2017

View reviewed changes

matt-gadd approved these changes Sep 12, 2017

View reviewed changes

tomdye added 2 commits September 12, 2017 15:32

uncomment tests

1e7cbd6

move projector emit, remove event typing, revert maps

5fdcf8b

agubler suggested changes Sep 13, 2017

View reviewed changes

tomdye added 2 commits September 14, 2017 10:51

pr comments

b067ace

pr comments

2e80508

agubler approved these changes Sep 14, 2017

View reviewed changes

tomdye merged commit 0f7f45e into dojo:master Sep 14, 2017

dylans added this to the 2017.09 milestone Sep 14, 2017

agubler mentioned this pull request Sep 22, 2017

Intersection Observer Meta #696

Merged

3 tasks

Add evented Node handler to meta #666

Add evented Node handler to meta #666

Conversation

tomdye commented Sep 8, 2017 • edited Loading

matt-gadd Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler left a 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

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

agubler Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

tomdye Sep 8, 2017 • 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

agubler left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Sep 8, 2017 •

edited

Loading

matt-gadd Sep 8, 2017 •

edited

Loading

agubler Sep 8, 2017 •

edited

Loading

tomdye Sep 8, 2017 •

edited

Loading