isDomNode() is confusing

[Reinmar]

Introduced in (without a ticket or at least a commit message): ckeditor/ckeditor5-utils@fafd3d3.

It doesn't check if something is a node. It doesn't do much more than lodash's isNative() so I'd like to know why it was needed.

[oleq]

I'd like to know why it was needed.

/Users/oleq/CK/5/ckeditor5/packages/ckeditor5-ui/src/template.js:
   17  import cloneDeepWith from '@ckeditor/ckeditor5-utils/src/lib/lodash/cloneDeepWith';
   18  import isObject from '@ckeditor/ckeditor5-utils/src/lib/lodash/isObject';
   19: import isDomNode from '@ckeditor/ckeditor5-utils/src/dom/isdomnode';
   20  import log from '@ckeditor/ckeditor5-utils/src/log';
   21  
   ..
  704  					container.appendChild( child.element );
  705  				}
  706: 			} else if ( isDomNode( child ) ) {
  707  				container.appendChild( child );
  708  			} else {
  ...
 1178  			} else {
 1179  				for ( const child of def.children ) {
 1180: 					if ( isTemplate( child ) || isView( child ) || isDomNode( child ) ) {
 1181  						children.push( child );
 1182  					} else {

/Users/oleq/CK/5/ckeditor5/packages/ckeditor5-utils/src/dom/emittermixin.js:
   11  import uid from '../uid';
   12  import extend from '../lib/lodash/extend';
   13: import isDomNode from './isdomnode';
   14  
   15  /**
   ..
   57  		// Check if emitter is an instance of DOM Node. If so, replace the argument with
   58  		// corresponding ProxyEmitter (or create one if not existing).
   59: 		if ( isDomNode( emitter ) ) {
   60  			args[ 0 ] = this._getProxyEmitter( emitter ) || new ProxyEmitter( emitter );
   61  		}
   ..
   86  
   87  		// Check if emitter is an instance of DOM Node. If so, replace the argument with corresponding ProxyEmitter.
   88: 		if ( isDomNode( emitter ) ) {
   89  			const proxy = this._getProxyEmitter( emitter );
   90  

It doesn't check if something is a node.

window instanceof Node; // -> false

And DOMEmitterMixin must be able to listen to events from window.

---

I think the only thing confusing here is the name of the method. It does suggest a comparison against Node, while it also includes window.

[pjasiun]

I agree that it's very strange that method called isDomNode return true for something that is not a DOM Node. For sure a better name should be provided (isNative, isDom?).

[oleq]

Maybe we should simply

1. leave the name,
2. correct the code to obj instanceof Node,
3. and make the DOMEmitterMixin use it in conjunction with isWindow() util?

[Reinmar]

obj instanceof Node is a no go – it won't work with iframes.

[oleq]

obj instanceof Node is a no go – it won't work with iframes.

Then let's use something smarter but separate the util from isWindow.