Separate FindWrapper core and extended methods

[mzgoddard]

The PRs on the new FindWrapper methods (#53, #57, #59, #63) help show a strata in the FindWrapper methods. We can categorize the existing and proposed methods as "core" and "extended".

The core methods need knowledge of the internal structure of FindWrapper to operate.

The extended methods can be implemented with the core methods and without internal knowledge of FindWrapper.

This can provide some benefits to preact-render-spy:

• Make FindWrapper's internal surface area smaller and future changes less prone to regressions on the extended methods
• Extended method tests do not need to test for error cases that core methods handle for them, making testing extended methods easier, easier to review, and hopefully easier to contribute.
• Help distinguish maintenance burden: changes to core methods should receive more thought and bugs receive higher priority
• Easier to accept new extended methods. They should still fill some user need.

The following actions need to be performed:

• [ ] Identify core and extended methods
• [ ] Determine a "mixin" strategy
  • How is FindWrapper "extended"?
  • How can extended methods be disabled in tests?
  • Should each extended method have a separate file and test file?
• [ ] Test that core without access to extended methods
• [ ] Determine if extended methods can depend on other extended methods
• [ ] Move core methods above extended methods in the documentation

[mzgoddard]

Here are the current and proposed core and extended methods.

core

• find
• attrs
• length property
• component
• output
• simulate
• setState

extended

• at
• children
• childAt #68
• filter
• attr
• text
• contains
• exists
• state

proposed

• name #57
• map #53
• reduce #63
• first #59
• last #59

[gnarf]

I'm a little lost imagining what this would look like, do you think you could whip up a quick aircode example of what you mean with one or two of the methods in a comment?

[mzgoddard]

I think if we add a Symbol.iterator and walk method, it opens a lot of the extended methods to work with Array.from or simple for-of loops.

This example got away from me. :wink:

// preact-render-spy.js
class FindWrapper {
  *[Symbol.iterator]() {
    for (let i = 0; i < this.length; i++) {
      yield this[i];
    }
  }

  *walk() {
    const vdomMap = this.context.vdomMap;
    for (const value of this) {
      yield value;
      if (typeof value.nodeName === 'function' && vdomMap.has(value)) {
        yield* this.find(vdom);
      }
      else {
        for (const child of (value.children || [])) {
          yield* this.find(child);
        }
      }
    }
  }

  find(selector) {
    verifyFoundNodes(this);
    if (typeof selector === 'object') {
      const _this = this;
      const unwrap = function*(iter) {
        for (const vdom of iter) {
          if (vdom instanceof FindWrapper) {
            yield vdom;
          }
          else {
            verifyContextHas(_this, vdom);
            yield vdom;
          }
        }
      };
      if (
        selector.hasProperty('length') ||
        selector.hasProperty(Symbol.iterator)
      ) {
        return new FindWrapper(this.context, unwrap(selector));
      }
      else {
        return new FindWrapper(this.context, unwrap([selector]));
      }
    }
    else {
      return new FindWrapper(this.context, this, selector);
    }
  }
}

mixin(FindWrapper, findWrapperExtended);

// find-wrapper-extended.js
module.exports = {
  contains: require('./find-wrapper-extended.contains').contains,
  map: require('./find-wrapper-extended.map').map,
};

// find-wrapper-extended.contains.js
const isEqual = require('lodash.isequal');

const contains = function(vdom) {
  for (const value of this.walk()) {
    if (isEqual(vdom, value)) {
      return true;
    }
  }
  return false;
};

contains.verify = {
  foundNodes: true,
};

module.exports = {contains};

// find-wrapper-extended.map.js
const map = function(fn, _this) {
  let index = 0;
  return Array.from(this, vdom => fn.call(_this, this.find(vdom), index++));
};

map.verify = {
  foundNodes: true,
};

module.exports = {map};

[mzgoddard]

@gnarf I don't think we need to stall #53 #57 #59 #63 on this issue. I think those PRs provide enough that we can continue discussion and merging of those without depending on this. (This comment isn't saying they are ready for merging I'll leave any thoughts I have on each separately.)

[mzgoddard]

I'm not sure about how to handle verify semantics. It feels like something that we may be able to use as metadata outside of just testing.

Maybe we expose the verify methods on the Wrapper instance?