Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional type checks to adapters #1701

Merged
merged 6 commits into from
Jul 22, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jul 2, 2018

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

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

return typeof node.type === 'function' ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left this since displayNameOfNode returns the display over the ctor name

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

export function nodeHasType(node, type) {
if (!type || !node) return false;

const displayName = getAdapter().displayNameOfNode(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i think this needs the same treatment

@@ -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]';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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

export function typeName(node) {
return typeof node.type === 'function'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

export function displayNameOfNode(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i think we may need to keep this as well

// 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') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a fallback for when the adapter lacks .isValidElementType

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

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

ljharb commented Jul 8, 2018

@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
Copy link
Contributor Author

jquense commented Jul 8, 2018

@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
Copy link
Member

ljharb commented Jul 8, 2018

@jquense I certainly understand :-) thanks!

@ljharb ljharb force-pushed the adapter-is-valid-element-type branch from 8480406 to fee9fa5 Compare July 8, 2018 06:16
@jquense jquense force-pushed the adapter-is-valid-element-type branch from fee9fa5 to e9e63a2 Compare July 9, 2018 13:45
@ljharb ljharb mentioned this pull request Jul 21, 2018
@jquense
Copy link
Contributor Author

jquense commented Jul 21, 2018

Sorry, this has tests now

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -801,4 +801,69 @@ describe('Adapter', () => {
},
}));
});

it.only('should determine valid element types', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, the only needs to be removed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


expect(adapter.isValidElementType('div')).to.equal(true);
expect(adapter.isValidElementType(Component)).to.equal(true);
expect(adapter.isValidElementType(ArrowFunction )).to.equal(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

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

if (createPortal) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

ljharb commented Jul 21, 2018

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 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
Copy link
Member

ljharb commented Jul 22, 2018

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

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

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jquense
Copy link
Contributor Author

jquense commented Jul 22, 2018

🙇 thanks for the help @ljharb

@ljharb ljharb force-pushed the adapter-is-valid-element-type branch from 6a47051 to 67b9dc0 Compare July 22, 2018 18:04
@ljharb ljharb merged commit 67b9dc0 into enzymejs:master Jul 22, 2018
ljharb added a commit that referenced this pull request Aug 25, 2018
 - 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 added a commit that referenced this pull request Aug 25, 2018
 - 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
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [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
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants