chore(expect): add exports to satisfy playwright

[mrienstra]

Summary

The playwright repo uses expect, I'd like to bump the version they're using from 26.4.2 to 27.1.0. Here is the issue which is motivating that version bump: #11640

You can see my proposed changes here: microsoft/playwright#8718
That PR is still marked as "draft", hoping to land this one first.

Playwright npm run check-deps runs utils/check_deps.js, which contains a function allowExternalImport / alllowExternalImport (very recently added in microsoft/playwright#8501), which has a problem with expect's package.json exports, thus the motivation for this PR.

---

Context on exports: added in PR #9921 and not modified since.

[facebook-github-bot]

Hi @mrienstra!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[mrienstra]

If "./build/*": "./build/*.js" is problematic, this would also resolve the issue with playwright's utils/check_deps.js:

"exports": {
    ".": "./build/index.js",
    "./package.json": "./package.json",
    "./build/jasmineUtils": "./build/jasmineUtils.js",
    "./build/matchers": "./build/matchers.js",
    "./build/print": "./build/print.js",
    "./build/types": "./build/types.d.ts",
    "./build/utils": "./build/utils.js"
},

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[SimenB]

Hey! 👋 I don't want to expose stuff from our build folder. I might be convinced to export whatever it is playwright needs, though. Do you have a list of what they use from expect?

[mrienstra]

Do you have a list of what they use from expect?

@SimenB:

src/test/expect.ts#L41

import matchers from 'expect/build/matchers';

---

src/test/matchers/toEqual.ts#L17-L20

import { equals } from 'expect/build/jasmineUtils';
import { iterableEquality } from 'expect/build/utils';

build/utils is already in exports, see packages/expect/package.json#L15

---

src/test/matchers/toMatchSnapshot.ts#L20

import { SyncExpectationResult } from 'expect/build/types';

---

src/test/matchers/toMatchText.ts#L17-L20

import {
  printReceivedStringContainExpectedResult,
  printReceivedStringContainExpectedSubstring
} from 'expect/build/print';

---

types/testExpect.d.ts#L10

import type { ExpectedAssertionsErrors } from 'expect/build/types';

---

Summary:

from 'expect/build/jasmineUtils' import
  { equals }
from 'expect/build/matchers' import
  matchers
from 'expect/build/print' import
  { 
    printReceivedStringContainExpectedResult,
    printReceivedStringContainExpectedSubstring
  }
from 'expect/build/types' import
  {
    ExpectedAssertionsErrors,
    SyncExpectationResult
  }
from 'expect/build/utils' import
  { iterableEquality }

[SimenB]

equals is injected as this.equals into all matchers, so that's not needed. (https://jestjs.io/docs/next/expect#thisequalsa-b)

expect/build/utils is already there as you say - we use it in jest-snapshot. Should probably fix that somehow...

SyncExpectationResult (and its brethren) I'm fine to export directly from expect, same with ExpectedAssertionsErrors (and any other potentially useful types).

printReceivedStringContainExpected* we can bundle those up in a printUtils or whatever and export that possibly. Not sure how easy exporting non-types are since we're stuck on exports = (CJS style)

---

Up to sending a PR for that?

[mrienstra]

equals is injected as this.equals into all matchers, so that's not needed. (https://jestjs.io/docs/next/expect#thisequalsa-b)

I suspect that's not going to work, given the way src/test/matchers/toEqual.ts#L46-L73 is using equals.

Up to sending a PR for that?

I may need a little assistance (similar PRs to use as a reference?), but certainly down to give it a shot. Lemmie try to get someone from playwright in here to make sure I'm accurately representing their interests / that we're on the right track from their perspective.

I could be way off here, but I'm starting to think some of the ways playwright reaches into expect may be atypical, and that they may be willing to accept -- as a side effect -- the rug could be pulled out from under them occasionally by updates as they're venturing beyond the public interface. In which case, they may simply wish to patch expect's "exports" to let them grab what they wish from ./build. Though I'm hearing you're amenable to exposing somewhere between almost everything and everything they need, which would surely be preferable... Just wanted to put that out there.

@pavelfeldman, @dgozman, @aslushnikov, @yury-s, @JoelEinbinder, @mxschmitt
(no pressure to respond if this isn't relevant to you, just casting a wide net)

[SimenB]

equals is injected as this.equals into all matchers, so that's not needed. (jestjs.io/docs/next/expect#thisequalsa-b)

I suspect that's not going to work, given the way src/test/matchers/toEqual.ts#L46-L73 is using equals.

They use this.isNot etc right above, can just capture this.equals there (seems it's all arrow functions, so just doing this.equals should work fine). I also noticed they import jest-matcher-utils there - they can access that off of this.utils and remove the dependency 🙂

I may need a little assistance (similar PRs to use as a reference?), but certainly down to give it a shot. Lemmie try to get someone from playwright in here to make sure I'm accurately representing their interests / that we're on the right track from their perspective.

Not sure we have similar PRs, but it should be enough to add here: https://github.com/facebook/jest/blob/90d6908492d164392ce8429923e7f0fa17946d2d/packages/expect/src/index.ts#L427-L430

At least type exports, not sure about the utils due to export = ... Might be convinced to put together a file containing any utils they need and have a separate supported file for that - reaching into build will never be supported.

We want expect to be usable in all use cases where an assertion library fits, so I'm definitely open to changing the package to make that easier 🙂

[mrienstra]

They use this.isNot etc right above, can just capture this.equals there (seems it's all arrow functions, so just doing this.equals should work fine).

Ah, right you are! Tried this locally.

I'll look into the rest later. Thanks!!!

[mrienstra]

Looking ahead to a post-microsoft/playwright#8782 playwright (thanks @JoelEinbinder!), problematic imports will be reduced to:

src/test/expect.ts#L41

import matchers from 'expect/build/matchers';

---

src/test/matchers/toEqual.ts#L17-L19

import { iterableEquality } from 'expect/build/utils';

---

src/test/matchers/toMatchText.ts#L17-L20

import {
  printReceivedStringContainExpectedResult,
  printReceivedStringContainExpectedSubstring
} from 'expect/build/print';

[SimenB]

We should definitely export the matchers somehow. Same with printing utils. Not sure about iterableEquality, seems edge-casey...

[codecov-commenter]

Codecov Report

Merging #11816 (142f85f) into main (03ff659) will decrease coverage by 0.01%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main   #11816      +/-   ##
==========================================
- Coverage   68.95%   68.93%   -0.02%     
==========================================
  Files         312      312              
  Lines       16400    16395       -5     
  Branches     4750     4750              
==========================================
- Hits        11308    11302       -6     
- Misses       5065     5066       +1     
  Partials       27       27              

Impacted Files	Coverage Δ
packages/expect/src/print.ts	97.61% <96.87%> (-0.35%)	⬇️
packages/expect/src/index.ts	92.00% <100.00%> (+0.10%)	⬆️
packages/expect/src/matchers.ts	97.86% <100.00%> (ø)
packages/expect/src/toThrowMatchers.ts	93.56% <100.00%> (ø)
packages/expect/src/utils.ts	95.58% <0.00%> (-0.56%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03ff659...142f85f. Read the comment docs.

[mrienstra]

We should definitely export the matchers somehow. Same with printing utils. Not sure about iterableEquality, seems edge-casey...

Re: iterableEquality: We could punt on this until you're ready to tackle removing ./build/utils from exports (as you mentioned here)

• Added expect.matchers in 65912a8
• Added expect.print in e3140f5

expect.print only includes the two methods playwright needs (printReceivedStringContainExpectedResult, printReceivedStringContainExpectedSubstring), which I have mixed feelings about, maybe it would be better to include all 8? If so, thoughts on minimizing type duplication (between packages/expect/src/print.ts & packages/expect/src/types.ts)?

[Edit: see below for a different approach I tried in 142f85f]

Here are the types for reference. All unique except for PrintExpectedConstructorName & PrintExpectedConstructorNameNot.

export type PrintReceivedStringContainExpectedSubstring = (
  received: string,
  start: number,
  length: number,
) => string;

export type PrintReceivedStringContainExpectedResult = (
  received: string,
  result: RegExpExecArray | null,
) => string;

export type PrintReceivedArrayContainExpectedItem = (
  received: Array<unknown>,
  index: number,
) => string;

export type PrintCloseTo = (
  receivedDiff: number,
  expectedDiff: number,
  precision: number,
  isNot: boolean,
) => string;

export type PrintExpectedConstructorName = (
  label: string,
  expected: Function,
) => string;

export type PrintExpectedConstructorNameNot = (
  label: string,
  expected: Function,
) => string;

export type PrintReceivedConstructorName = (
  label: string,
  received: Function,
) => string;

export type PrintReceivedConstructorNameNot = (
  label: string,
  received: Function,
  expected: Function,
) => string;

[mrienstra]

Extended expect.print in 142f85f to include all 8 methods from packages/expect/src/print.ts.

Types for these are now all in packages/expect/src/types.ts.

Made a print object to export, in part so the types can be bundled as PrintObject, but open to doing it differently.

Downside obviously is more changes.

Changes to packages/expect/src/print.ts are hard to skim (the GitHub diff is even worse than the VS Code diff), as I had to alphabetize the order of the methods for eslint sort-keys, and I chose to move printConstructorName up to below printSubstring (the other non-exported function).

Because types moved to packages/expect/src/types.ts, readability is reduced, before you could see types inline (top screenshot below), now (in VS Code) you must hover over the method name (or a parameter, or the =>) to see the types (bottom screenshot below).

[SimenB]

yeah, not a huge fan as it doesn't really belong on the expect function... We might need to defer this to the next major when we can migrate to ESM style exports (still transpiled to CJS, tho).

One alternative is to create a new package (@jest/expect-utils or some such) which exports them for public consumption and use within expect itself. Not a huge fan of that either TBH 😅 So this might need to wait for Jest 28

[mrienstra]

Fair enough!

After it kicks around the back of your brain a bit more, let me know if you see another way forward. In the meantime, if necessary, consumers can always patch. 🩹

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