Add additional type checks to adapters #1701

jquense · 2018-07-02T14:23:47Z

Required for #1592 to work properly, still in progress but i want to make sure the proposed API changes to adapters are ok before i polish up all the tests and what not

cc @ljharb

…ValidElementType`

… if available

…fNode`, if present

…, if present

…entType`, if present

jquense · 2018-07-02T14:25:03Z

packages/enzyme/src/Utils.js

-  return (typeof node.type === 'function' ?
-    functionName(node.type) === type : node.type.name === type) || node.type.displayName === type;
+
+  return typeof node.type === 'function' ?


left this since displayNameOfNode returns the display over the ctor name

ljharb

I'm concerned about how to make this not be a breaking change - specifically, with "old enzyme + new adapter" ("new enzyme + old adapter" and "new enzyme + new adapter" should work fine)

ljharb · 2018-07-04T00:44:23Z

packages/enzyme/src/ReactWrapper.js

@@ -728,7 +727,8 @@ class ReactWrapper {
   * @returns {String}
   */
  name() {
-    return this.single('name', n => displayNameOfNode(n));
+    const adapter = getAdapter(this[OPTIONS]);
+    return this.single('name', n => adapter.displayNameOfNode(n));


i think this needs adapter.displayNameOfNode ? adapter.displayNameOfNode(n) : displayNameOfNode(n)

ljharb · 2018-07-04T00:44:41Z

packages/enzyme/src/ShallowWrapper.js

@@ -896,7 +895,8 @@ class ShallowWrapper {
   * @returns {String}
   */
  name() {
-    return this.single('name', displayNameOfNode);
+    const adapter = getAdapter(this[OPTIONS]);
+    return this.single('name', n => adapter.displayNameOfNode(n));


i think this needs adapter.displayNameOfNode ? adapter.displayNameOfNode(n) : displayNameOfNode

ljharb · 2018-07-04T00:44:59Z

packages/enzyme/src/Utils.js

@@ -42,10 +42,14 @@ export function typeOfNode(node) {

 export function nodeHasType(node, type) {
  if (!type || !node) return false;
+
+  const displayName = getAdapter().displayNameOfNode(node);


and i think this needs the same treatment

ljharb · 2018-07-04T00:45:38Z

packages/enzyme/src/Debug.js

@@ -66,6 +64,7 @@ function indentChildren(childrenStrs, indentLength) {

 export function debugNode(node, indentLength = 2, options = {}) {
  if (typeof node === 'string' || typeof node === 'number') return escape(node);
+  if (typeof node === 'function') return '[function]';


a) this should probably be [function Foo] where Foo is the functionName
b) could this small change be peeled out into a separate PR with its own tests?

ljharb · 2018-07-04T00:46:02Z

packages/enzyme/src/Debug.js


 const booleanValue = Function.bind.call(Function.call, Boolean.prototype.valueOf);

-export function typeName(node) {
-  return typeof node.type === 'function'


i think all this logic needs to stay as a fallback for when the adapter lacks .displayNameOfNode

ljharb · 2018-07-04T00:46:39Z

packages/enzyme/src/Utils.js

@@ -214,15 +218,6 @@ export function AND(fns) {
  return x => fnsReversed.every(fn => fn(x));
 }

-export function displayNameOfNode(node) {


and i think we may need to keep this as well

ljharb · 2018-07-04T00:47:06Z

packages/enzyme/src/selectors.js

-  // If the selector is a function, check if the node's constructor matches
-  if (typeof selector === 'function') {
+  // If the selector is a string, parse it as a simple CSS selector
+  if (typeof selector === 'string') {


these changes also seem like they could be peeled out into a separate PR?

They could yeah, I wanted to validate that the apis I was adding were sufficient

ljharb · 2018-07-04T00:47:21Z

packages/enzyme/src/selectors.js

+    return buildPredicateFromToken(tokens[0]);
+  }
+  // If the selector is an element type, check if the node's type matches
+  if (getAdapter().isValidElementType(selector)) {


this needs a fallback for when the adapter lacks .isValidElementType

ljharb

ok, i rebased this a bit and split up some commits. what this needs now is tests :-)

ljharb · 2018-07-08T00:22:15Z

@jquense any chance you're able to write some tests for this PR, so we can get it in (and then we can rebase #1592 and get it in)?

jquense · 2018-07-08T00:24:49Z

@ljharb I'm hoping to have a moment, i'm pretty swamped with life and work at the moment but may have a second to finish this up in the next few days

ljharb · 2018-07-08T00:32:58Z

@jquense I certainly understand :-) thanks!

jquense · 2018-07-21T12:29:28Z

Sorry, this has tests now

ljharb

Thanks! There’s a few more tweaks needed; if you don’t have a chance to update the PR, I’ll be happy to do it for you.

ljharb · 2018-07-21T14:03:10Z

packages/enzyme-test-suite/test/Adapter-spec.jsx

@@ -801,4 +801,69 @@ describe('Adapter', () => {
      },
    }));
  });
+
+  it.only('should determine valid element types', () => {


oops, the only needs to be removed

ljharb · 2018-07-21T14:03:35Z

packages/enzyme-test-suite/test/Adapter-spec.jsx

+    class Component extends React.Component {
+      render() { return null }
+    }
+    const ArrowFunction = () => null


the arrow function test needs it’s own it, so it can be skipped on React 13

ljharb · 2018-07-21T14:03:42Z

packages/enzyme-test-suite/test/Adapter-spec.jsx

+
+    expect(adapter.isValidElementType('div')).to.equal(true);
+    expect(adapter.isValidElementType(Component)).to.equal(true);
+    expect(adapter.isValidElementType(ArrowFunction )).to.equal(true);


extra space

ljharb · 2018-07-21T14:04:27Z

packages/enzyme-test-suite/test/Adapter-spec.jsx

+    expect(adapter.isValidElementType(Component)).to.equal(true);
+    expect(adapter.isValidElementType(ArrowFunction )).to.equal(true);
+
+    if (createPortal) {


instead of using if checks, these should be checking the React versions and running conditionslly, as their own it’s

ljharb · 2018-07-21T20:50:01Z

I've done some rebasing; this PR now has 6 commits, the first 2 of which are (thoroughly) tested. I can pull those into master now, but the last 4 commits still need test coverage.

ljharb · 2018-07-22T04:31:17Z

i've added some more tests; now of the 6 commits, ~~the first 4~~ all but the last have sufficient tests.

ljharb

k, that should do it. I'll merge in the morning once tests pass.

jquense · 2018-07-22T13:41:16Z

🙇 thanks for the help @ljharb

@jquense

- Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense) - pass the adapter into `createMountWrapper` (#1592, @jquense) - update deps - [meta] ensure a license and readme is present in all packages when published

@ljharb

- Add pointer events support (#1753, @ljharb) - Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense) - pass the adapter into `createMountWrapper` (#1592, @jquense) - update deps - [meta] ensure a license and readme is present in all packages when published

@jgzuke

- [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke) - [New] Add pointer events support (#1753, @ljharb) - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense) - [New] pass the adapter into `createMountWrapper` (#1592, @jquense) - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04) - update deps - [meta] ensure a license and readme is present in all packages when published

@jquense

- [New] Add forwardRef support (#1592, @jquense) - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke) - [New] Add pointer events support (#1753, @ljharb) - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense) - [New] pass the adapter into `createMountWrapper` (#1592, @jquense) - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary) - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04) - update deps - [meta] ensure a license and readme is present in all packages when published

jquense added 6 commits July 2, 2018 10:22

[enzyme-adapter-utils] [New] add displayNameOfNode

6c5e6ba

[enzyme-adapter-react-{13,14,15.4,15,16] Add displayNameOfNode, `is…

e163a35

…ValidElementType`

[New] Debug: typeName now calls the adapter’s displayNameOfNode…

c401fde

… if available

[New] mount/shallow: .name(): call into adapter’s `displayNameO…

3db6d2c

…fNode`, if present

[New] Utils: nodeHasType: call into adapter’s displayNameOfNode…

02f6f21

…, if present

[New] selectors: buildPredicate: call into adapter’s `isValidElem…

67b9dc0

…entType`, if present

jquense commented Jul 2, 2018

View reviewed changes

jquense mentioned this pull request Jul 2, 2018

[react-is] Add elementType API facebook/react#12932

Closed

ljharb reviewed Jul 4, 2018

View reviewed changes

ljharb force-pushed the adapter-is-valid-element-type branch from 091fc3e to d79da18 Compare July 5, 2018 22:38

ljharb reviewed Jul 5, 2018

View reviewed changes

ljharb force-pushed the adapter-is-valid-element-type branch from d79da18 to 8480406 Compare July 6, 2018 00:27

ljharb added this to Triage in Shallow/Mount wrapper identity difference Jul 6, 2018

ljharb removed this from Triage in Shallow/Mount wrapper identity difference Jul 6, 2018

ljharb force-pushed the adapter-is-valid-element-type branch from 8480406 to fee9fa5 Compare July 8, 2018 06:16

jquense force-pushed the adapter-is-valid-element-type branch from fee9fa5 to e9e63a2 Compare July 9, 2018 13:45

ljharb added this to Needs Triage in React 16 Jul 10, 2018

ljharb mentioned this pull request Jul 21, 2018

Forward ref #1592

Merged

ljharb requested changes Jul 21, 2018

View reviewed changes

ljharb force-pushed the adapter-is-valid-element-type branch from e9e63a2 to 1a6b47b Compare July 21, 2018 20:49

ljharb force-pushed the adapter-is-valid-element-type branch 2 times, most recently from b2743d8 to 6824acf Compare July 22, 2018 04:30

ljharb force-pushed the adapter-is-valid-element-type branch 2 times, most recently from ee6ce05 to bdec3d4 Compare July 22, 2018 05:36

ljharb added API: shallow API: mount semver: minor New stuff. package: react adapter: 16 package: react adapter: 15 package: react adapter: 15.4 package: react adapter: 0.14 package: react adapter: 0.13 package: enzyme-adapter-utils labels Jul 22, 2018

ljharb self-assigned this Jul 22, 2018

ljharb changed the title ~~[WIP] Add additional type checks to adapters~~ Add additional type checks to adapters Jul 22, 2018

ljharb force-pushed the adapter-is-valid-element-type branch from bdec3d4 to 6a47051 Compare July 22, 2018 05:46

ljharb approved these changes Jul 22, 2018

View reviewed changes

ljharb force-pushed the adapter-is-valid-element-type branch from 6a47051 to 67b9dc0 Compare July 22, 2018 18:04

ljharb merged commit 67b9dc0 into enzymejs:master Jul 22, 2018

This was referenced Oct 1, 2018

Update enzyme in /modules/frontend from 3.3.0 to 3.6.0 project-accelerate/accelerate#265

Open

Update enzyme in / from 3.3.0 to 3.6.0 leonardofaria/leozera-ui#234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional type checks to adapters #1701

Add additional type checks to adapters #1701

jquense commented Jul 2, 2018

jquense Jul 2, 2018

ljharb left a comment

ljharb Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

jquense Jul 4, 2018

ljharb Jul 4, 2018

ljharb left a comment

ljharb commented Jul 8, 2018

jquense commented Jul 8, 2018

ljharb commented Jul 8, 2018

jquense commented Jul 21, 2018

ljharb left a comment

ljharb Jul 21, 2018

ljharb Jul 21, 2018

ljharb Jul 21, 2018

ljharb Jul 21, 2018

ljharb commented Jul 21, 2018

ljharb commented Jul 22, 2018 •

edited

ljharb left a comment

jquense commented Jul 22, 2018

Add additional type checks to adapters #1701

Add additional type checks to adapters #1701

Conversation

jquense commented Jul 2, 2018

Choose a reason for hiding this comment

ljharb 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

ljharb left a comment

Choose a reason for hiding this comment

ljharb commented Jul 8, 2018

jquense commented Jul 8, 2018

ljharb commented Jul 8, 2018

jquense commented Jul 21, 2018

ljharb 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

ljharb commented Jul 21, 2018

ljharb commented Jul 22, 2018 • edited

ljharb left a comment

Choose a reason for hiding this comment

jquense commented Jul 22, 2018

ljharb commented Jul 22, 2018 •

edited