Check for existence of Element when comparing DOM-nody-things (#7786)

[mfeineis]

Summary

Comparing things that look like DOM nodes now checks if Element is in scope before usage which would lead to ReferenceError: Element is not defined errors with 'testEnvironment': 'node'.

Test plan

/**
 * @jest-environment node
 */
describe('comparing DOM-nody-things', () => {
  it('should skip the "instanceof Element" check when "Element" is not in scope', () => {
    expect({ nodeName: "div", nodeType: 1 }).toEqual({ nodeName: "div", nodeType: 1 });
  });
});

First contribution and signed the CLA.

[pedrottimark]

Thank you for first contribution to Jest!

In plain node testing environment where Element doesn’t exist, what is the criterion? generic deep equality for properties of expected and received as ordinary objects?

Can you add a test to the positive .toEqual failure snapshots above the .not.toEqual tests?

    [
      {
        nodeName: 'div',
        nodeType: 1,
      },
      {
        nodeName: 'p',
        nodeType: 1,
      },
    ]

It looks like that test might not succeed (by failing :) because fall through to:

    return a.innerText == b.innerText && a.textContent == b.textContent;

Let’s make sure if any code change is needed, it doesn’t break original test because fall through to:

  if (aIsDomNode || bIsDomNode) {
    return false;
  }

[mfeineis]

Oops, didn't catch that :-).

If you take a look at the current jasmine toEqual implementation they skipped the instanceof Element check on site https://github.com/jasmine/jasmine/blob/master/src/core/matchers/matchersUtil.js#L181 but the isDomNode helper checks for a Node constructor on the global object https://github.com/jasmine/jasmine/blob/master/src/core/base.js#L103

Judging from the comments the code path only seems to be important for IE<9 inside a browser which is probably not a supported target of Jest anyway? Just skipping the whole DOM node detection section if Element isn't in scope and let the deep comparison do the rest should be OK I think. Although that might be problematic if react also relies on that path for its DOM stuff. I'm kind of surprised that I'm the first to stumble upon this ^.^

The latest commit leaves the duck typing in place and lets me (or other fiddlers) reimplement isEqualNode in my fake DOM nodes if I choose to do so but it will skip the IE8 stuff if Element isn't in scope and also skips the early return in that case, what do you think?

[SimenB]

We should support ie11 (for now), so feel free to remove compat code for older browsers

[pedrottimark]

So we assume that isDomNode method is available in supported browser testing environments.

I will make sure whether or not isDomNode is available in jsdom testing environment.

@mfeineis in your node testing environment without jsdom, is generic deep equality comparison of properties the desired criterion for objects which match the isDomNode heuristic?

What do you think about cutting the special case to the minimum:

  if (isDomNode(a) && isDomNode(b) && typeof a.isEqualNode === 'function') {
    return a.isEqualNode(b);
  }

[mfeineis]

Yes for my case these false positives should just be handled as plain objects because they are just that. I did something very similar what you suggested in my latest commit, it might be OK to leave out the if (typeof Element !== 'undefined' && (aIsDomNode || bIsDomNode)) { return false; } early return although that might have performance implications. The test suite seems to work on my system, although CircleCI isn't happy on windows apparently.

[pedrottimark]

Yes, it gave me a smile when your commit and my comment crossed paths.

I think it is clearer without the second if statement for node and non-node.

Just verified that element and text node have isEqualNode method in jsdom.

Oh, what do you think about adding isEqualNode method to heuristic function:

function isDomNode(obj) {
  return (
    obj !== null &&
    typeof obj === 'object' &&
    typeof obj.nodeType === 'number' &&
    typeof obj.nodeName === 'string' &&
    typeof obj.isEqualNode === 'function'
  );
}

  if (isDomNode(a) && isDomNode(b)) {
    return a.isEqualNode(b);
  }

[mfeineis]

Merging the isEqualNode into isDomNode changes the semantics but may be the way to go, I will try that later today. I can't think of a sensible use-case where it makes sense to compare DOM nodes to something else, could still be costly, though.

[pedrottimark]

Yes, good point that shortcut return reduces cost of comparing.

It is super for this pull request to fix the problem, delete obsolete code, and keep same semantics.

[mfeineis]

I thought about it some more. I think jasmine's current solution is a good one: include the instanceof Node check in isDomNode, keep the check for isEqualNode and remove the early return because we can't really tell if these are real ones so we avoid false negatives. If we have a real DOM node or something DOM-Level-3 compliant it'll just use the isEqualNode which should be the overwhelming majority and for cases like mine deep equality should work but may of course be slower. What do you think?

[pedrottimark]

Thank you for thinking about it some more. Two heads are better than one.

Here is my short summary of the solution: for two instances of Node which have DOM3 method isEqualNode then use it as special case; otherwise compare according to deep equality.

1. For Jest it seems safer to drop the duck type condition:

function isDomNode(obj) {
  return typeof Node !== 'undefined' && obj instanceof Node;
}

because if a is a Node from jsdom environment and b is duck type literal object:

TypeError: Failed to execute 'isEqualNode' on 'Node': parameter 1 is not of type 'Node'.

It doesn’t seem likely if a is duck type object that it would have isEqualNode method.

2. A style suggestion that you can take or leave is typeof a.isEqualNode === 'function'

Before we merge (but not today) I’ll write local tests comparing a as Node to b as duck type literal object in current baseline and proposed improved comparison to see if fall through seems too risky.

[mfeineis]

I've added the typeof a.isEqualNode === 'function', I wonder if that is even necessary anymore due to the hard check for instanceof Node without the duck typing, which I also added for good measure 😃

[pedrottimark]

Looks good to me!

[pedrottimark]

While we wait for #7919 conversion of expect package to TypeScript with hopefully not too many merge conflicts, here is what I thought might be a problem, and why it isn’t.

I thought because Object.keys returns empty array for DOM nodes, that without short-circuit:

  if (aIsDomNode || bIsDomNode) {
    return false;
  }

That the following tests would incorrectly pass:

expect(document.createElement('div')).toEqual({});
expect({}).toEqual(document.createElement('div'));

The reason that they correctly fail is an upstream short-circuit:

  var className = Object.prototype.toString.call(a);
  if (className != Object.prototype.toString.call(b)) {
    return false;
  }

For example:

'[object HTMLDivElement]' !== '[object Object]' 
'[object Object]' !== '[object HTMLDivElement]'

Also because '[object HTMLParagraphElement]' !== '[object HTMLDivElement]' different elements don’t always need .isEqualNode comparison.

Except beware that in jsdom that Jest uses at the time of writing, different SVG elements like path and polyline are instances of same SVGElement class.

After this PR has been merged, you or I might add a small matchers-dom.test.js with directives

 * @jest-environment jsdom
 */
/* eslint-env browser*/

as in for example, packages/pretty-format/src/tests/DOMElement.test.ts file

@SimenB @thymikee can you see any problem with the minimal code we kept for DOM elements?

[pedrottimark]

@mfeineis The expect package is now TypeScript, so you can merge changes from master into your branch, and then resolve any conflicts in jasmineUtils.ts file (your local git understands renaming, but if your editor doesn’t, then close renamed .js files without saving changes).

After so many changes in recent days: yarn clean-all && yarn before yarn test

[mfeineis]

ahh, shoot something went wrong...

[mfeineis]

I have a feeling I put my commits on the wrong end of the rebase-train but github shows the changes I'm expecting so hopefully I didn't break anything 😇

[mfeineis]

I could also spin up a new branch on top of master and cherry pick my changes on top if that is easier to manage.

[pedrottimark]

At my level of git knowledge, all’s well that ends well. Other collaborators can tell us if they see a problem. Ever since GitHub got Squash and merge option, you can take the term “rebase” loosely on Jest project (unlike some projects whose maintainers mean it literally). My recipe is:

git checkout master
git fetch upstream
git merge upstream/master
git checkout feature-branch
git merge master
# fix merge conflicts and commit
# continue with feature

[pedrottimark]

Sure, that sounds like a plan. That is the way that I have felt, when I have been you :)

[SimenB]

#7995

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.