[expect] Protect against undefined matcher when extending

[IanVS]

Summary

This fixes an issue when import * as matchers from 'my-matchers'; includes a default import whose value is undefined, as is happening in storybookjs/jest#15. It just adds a bit of safety to be sure that the matcher is defined, before trying to add it.

Test plan

I added a unit test which fails without this change.

[SimenB]

Thanks! Now the question is, should we throw a descriptive error, or ignore the user input?

I'm thinking we should throw, since it could be a genuine bug. E.g.

const {
  myFirstMatcher,
  // notice typo
  mySecondMachter,
} = require('./my-matchers');

expect.extend({
  FirstMatcher: myFirstMatcher,
  SecondMatcher: mySecondMachter,
});

Getting Error adding matcher 'SecondMatcher' - it is not defined or some such is better than later SecondMatcher is not a function

[IanVS]

Hmm, that's not really any better for my situation, however, since the reason I am submitting this PR is that I cannot remove the offending matcher. In my case, I am dealing with matchers that come from:

import * as matchers from '@testing-library/jest-dom/matchers';

I don't understand why (I think it is a cjs vs esm issue), but a default import with a value of undefined is also being included, which is causing an error to be thrown. See the issue I linked in the PR description. There seems to be no way to remove that default from a namespace import like this.

If this PR causes a different error to be thrown, then I'm back to square 1. Do you have any ideas what else could be done? Maybe a console warning instead?

[SimenB]

In my case, I am dealing with matchers that come from:

import * as matchers from '@testing-library/jest-dom/matchers';

Yeah, it's CJS so you can't really do that safely. Seems like a broken transpilation that gives named exports at all - native ESM gives a single default

$ node --input-type module --eval "import * as matchers from '@testing-library/jest-dom/matchers.js'; console.log(matchers)"
[Module: null prototype] {
  default: {
    toBeChecked: [Getter],
    toBeDisabled: [Getter],
    toBeEmpty: [Getter],
    toBeEmptyDOMElement: [Getter],
    toBeEnabled: [Getter],
    toBeInTheDOM: [Getter],
    toBeInTheDocument: [Getter],
    toBeInvalid: [Getter],
    toBePartiallyChecked: [Getter],
    toBeRequired: [Getter],
    toBeValid: [Getter],
    toBeVisible: [Getter],
    toContainElement: [Getter],
    toContainHTML: [Getter],
    toHaveAccessibleDescription: [Getter],
    toHaveAccessibleName: [Getter],
    toHaveAttribute: [Getter],
    toHaveClass: [Getter],
    toHaveDescription: [Getter],
    toHaveDisplayValue: [Getter],
    toHaveErrorMessage: [Getter],
    toHaveFocus: [Getter],
    toHaveFormValues: [Getter],
    toHaveStyle: [Getter],
    toHaveTextContent: [Getter],
    toHaveValue: [Getter]
  }
}

What you want is probably the default export

$ node --input-type module --eval "import matchers from '@testing-library/jest-dom/matchers.js'; console.log(matchers)"
{
  toBeChecked: [Getter],
  toBeDisabled: [Getter],
  toBeEmpty: [Getter],
  toBeEmptyDOMElement: [Getter],
  toBeEnabled: [Getter],
  toBeInTheDOM: [Getter],
  toBeInTheDocument: [Getter],
  toBeInvalid: [Getter],
  toBePartiallyChecked: [Getter],
  toBeRequired: [Getter],
  toBeValid: [Getter],
  toBeVisible: [Getter],
  toContainElement: [Getter],
  toContainHTML: [Getter],
  toHaveAccessibleDescription: [Getter],
  toHaveAccessibleName: [Getter],
  toHaveAttribute: [Getter],
  toHaveClass: [Getter],
  toHaveDescription: [Getter],
  toHaveDisplayValue: [Getter],
  toHaveErrorMessage: [Getter],
  toHaveFocus: [Getter],
  toHaveFormValues: [Getter],
  toHaveStyle: [Getter],
  toHaveTextContent: [Getter],
  toHaveValue: [Getter]
}

---

Since it works at all, but empty default, I assume your env is not really ESM but using Babel/TS. You can use createRequire to work around it probably, which'll work fine in both real and simulated ESM envs.

import {createRequire} from 'module';

const require = createRequire(import.meta.url)

const matchers = require('@testing-library/jest-dom/matchers')

[IanVS]

Thanks for the suggestions. My actual situation is that I am using vite and running (a polyfilled version of) expect in the browser. My project is importing @storybook/jest, which imports expect and extends it with jest-dom. The storybook module package publishes an ES module, but jest-dom does not. Storybook is building with tsc, as you guessed, which apparently is not behaving the same way as you showed above, because I did try importing the default export, but that did not work, it came back undefined. Because this is running in the browser, I don't believe I'll be able to use createRequire.

Maybe the real solution is for jest-dom to publish an ES module. I can work on a PR for them, but in the meantime I was hoping to get a quick easy win here with a safety check. I can continue to use patch-package to hack it in myself, but it's not a great solution for other vite storybook users.

[SimenB]

import * as matchers from '@testing-library/jest-dom/matchers' doesn't work in a browser anyways, so whatever does the bundling probably understands createRequire as well? I believe at least webpack does.

You could change to require I guess? Not sure how the bundling work, but at least webpack will handle that without issues.

Taking a step back - some layer of ESM source pulling in CJS is wrong though (guessing it's a bundler, as using ts-node and import * as matchers from '@testing-library/jest-dom/matchers' works fine without any default added (with default tsc --init plus "moduleResolution": "node")), not sure we should work around that in Jest.

Providing a better error than TypeError: Object.defineProperty called on non-object is definitely doable, tho!

[SimenB]

Something like

diff --git i/packages/expect/src/__tests__/extend.test.ts w/packages/expect/src/__tests__/extend.test.ts
index 852dfb139f..f0ff670601 100644
--- i/packages/expect/src/__tests__/extend.test.ts
+++ w/packages/expect/src/__tests__/extend.test.ts
@@ -184,3 +184,27 @@ it('allows overriding existing extension', () => {
 
   jestExpect('foo').toAllowOverridingExistingMatcher();
 });
+
+it('throws descriptive errors for invalid matchers', () => {
+  expect(() =>
+    jestExpect.extend({
+      default: undefined,
+    }),
+  ).toThrow(
+    'expect.extend: `default` is not a valid matcher. Must be function, is "undefined"',
+  );
+  expect(() =>
+    jestExpect.extend({
+      default: 42,
+    }),
+  ).toThrow(
+    'expect.extend: `default` is not a valid matcher. Must be function, is "number"',
+  );
+  expect(() =>
+    jestExpect.extend({
+      default: 'foobar',
+    }),
+  ).toThrow(
+    'expect.extend: `default` is not a valid matcher. Must be function, is "string"',
+  );
+});
diff --git i/packages/expect/src/jestMatchersObject.ts w/packages/expect/src/jestMatchersObject.ts
index 90e26e6b77..7aa76977df 100644
--- i/packages/expect/src/jestMatchersObject.ts
+++ w/packages/expect/src/jestMatchersObject.ts
@@ -6,6 +6,7 @@
  *
  */
 
+import {getType} from 'jest-get-type';
 import {AsymmetricMatcher} from './asymmetricMatchers';
 import type {
   Expect,
@@ -56,6 +57,15 @@ export const setMatchers = (
 ): void => {
   Object.keys(matchers).forEach(key => {
     const matcher = matchers[key];
+
+    if (typeof matcher !== 'function') {
+      throw new TypeError(
+        `expect.extend: \`${key}\` is not a valid matcher. Must be function, is "${getType(
+          matcher,
+        )}"`,
+      );
+    }
+
     Object.defineProperty(matcher, INTERNAL_MATCHER_FLAG, {
       value: isInternal,
     });

[IanVS]

FWIW, I think I found the root cause of the issue I'm experiencing: evanw/esbuild#1971 (comment)

[SimenB]

Sounds likely 👍

[SimenB]

The new validation is in https://github.com/facebook/jest/releases/tag/v28.0.0-alpha.5

[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.