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

chore(expect): add exports to satisfy playwright #11816

Closed
wants to merge 7 commits into from

Conversation

mrienstra
Copy link
Contributor

@mrienstra mrienstra commented Sep 5, 2021

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.

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 motivated this change: jestjs#11640

You can see my proposed changes here:
microsoft/playwright@master...mrienstra:patch-3

I haven't opened a PR for those changes yet, hoping to land this one first.

Playwright `npm 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 the `expect` `package.json` `exports`, thus the motivation for this PR.
@facebook-github-bot
Copy link
Contributor

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!

Copy link
Contributor Author

@mrienstra mrienstra left a comment

Choose a reason for hiding this comment

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

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

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

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

@mrienstra mrienstra changed the title chore: add exports to satisfy playwright chore(expect): add exports to satisfy playwright Sep 5, 2021
@facebook-github-bot
Copy link
Contributor

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

2 similar comments
@facebook-github-bot
Copy link
Contributor

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

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

@SimenB
Copy link
Member

SimenB commented Sep 6, 2021

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

mrienstra commented Sep 6, 2021

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

SimenB commented Sep 7, 2021

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

mrienstra commented Sep 8, 2021

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

SimenB commented Sep 8, 2021

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

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

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

SimenB commented Sep 9, 2021

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

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

Impacted file tree graph

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

mrienstra commented Sep 9, 2021

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)

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

mrienstra commented Sep 9, 2021

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).
comparison

Comment on lines +116 to +117
matchers: MatchersObject;
print: PrintObject;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2021
@mrienstra mrienstra deleted the patch-1 branch October 11, 2021 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants