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(packages): support pretty-dom esm build #964

Conversation

MatanBobi
Copy link
Member

@MatanBobi MatanBobi commented May 29, 2021

What:

This adds support for pretty-dom 27 as it's moved to export default only the format function.

Why:

This will fix a break for everyone using jest 27. Resolves #963

How:

Upgrade the dependency and import plugins where it's needed.

Checklist:

I'm pretty sure this shouldn't be a breaking change.

  • Documentation added to the
    docs site - N/A
  • Tests - N/A
  • Typescript definitions updated - N/A
  • Ready to be merged

@MatanBobi MatanBobi changed the title chore(packages): support pretty-dom esm modules build chore(packages): support pretty-dom esm build May 29, 2021
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 771c190:

Sandbox Source
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #964 (771c190) into main (c273ed5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #964   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          966       966           
  Branches       293       298    +5     
=========================================
  Hits           966       966           
Flag Coverage Δ
node-10.14.2 100.00% <100.00%> (ø)
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-15 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pretty-dom.js 100.00% <100.00%> (ø)

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 c273ed5...771c190. Read the comment docs.

@MatanBobi MatanBobi requested a review from eps1lon May 31, 2021 08:02
@eps1lon
Copy link
Member

eps1lon commented May 31, 2021

This will break for everyone uses jest 27.

This change will break or existing code does break? What happens to people still on jest 26?

@MatanBobi
Copy link
Member Author

This will break for everyone uses jest 27.

This change will break or existing code does break? What happens to people still on jest 26?

This change will fix the breaking in jest 27.
I still didn't get a chance to understand why it's breaking though if we're using a specific version of pretty-format. Based on this issue it looks like jest are using their own pretty-format and not the one in our dependencies.

If this is in fact the way it works, sounds like it will break for users using jest 26 (though it didn't fail our tests).
I'll try to link this version to a local project I have and see.

@eps1lon
Copy link
Member

eps1lon commented May 31, 2021

If this is in fact the way it works, sounds like it will break for users using jest 26 (though it didn't fail our tests).

That's my main confusion here. If this supposedly breaks jest 26, why doesn't it break our test suite? So let's find out first why #963 pulls in a different version of pretty-dom. Might be a package manager bug or issue with the bundling setup.

@lukeapage
Copy link

Do you want any more info from me?

$ yarn why pretty-format
yarn why v1.19.1
[1/4] Why do we have the module "pretty-format"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "pretty-format@27.0.1"
info Has been hoisted to "pretty-format"
info Reasons this module exists
   - "workspace-aggregator-039abf99-d701-42f4-a83c-b114a8390581" depends on it
   - Hoisted from "_project_#jest-diff#pretty-format"
   - Hoisted from "_project_#jest-circus#pretty-format"
   - Hoisted from "_project_#jest-cli#jest-config#pretty-format"
   - Hoisted from "_project_#jest-circus#jest-each#pretty-format"
   - Hoisted from "_project_#jest-circus#jest-matcher-utils#pretty-format"
   - Hoisted from "_project_#jest-circus#jest-message-util#pretty-format"
   - Hoisted from "_project_#jest-circus#jest-snapshot#pretty-format"
   - Hoisted from "_project_#jest-cli#jest-validate#pretty-format"
   - Hoisted from "_project_#jest-cli#jest-config#jest-jasmine2#pretty-format"
   - Hoisted from "_project_#jest-circus#jest-runner#jest-leak-detector#pretty-format"
info Disk size without dependencies: "164KB"
info Disk size with unique dependencies: "332KB"
info Disk size with transitive dependencies: "1.43MB"
info Number of shared dependencies: 14
=> Found "@types/jest#pretty-format@26.6.2"
info Reasons this module exists
   - "_project_#@types#jest" depends on it
   - Hoisted from "_project_#@types#jest#jest-diff#pretty-format"
info Disk size without dependencies: "140KB"
info Disk size with unique dependencies: "308KB"
info Disk size with transitive dependencies: "1.41MB"
info Number of shared dependencies: 14
=> Found "@testing-library/dom#pretty-format@26.6.2"
info This module exists because "_project_#@testing-library#react#@testing-library#dom" depends on it.

So, if node imports testing-library first, then it will cache v26 of pretty-format for all requests within that process. But if jest imports it first, it will cache v27 against node. I thought that was just the way node works - but maybe I am missing something that jest does?

When I add a test resolution to this branch to my jest v27 branch

"@testing-library/dom": "https://pkg.csb.dev/testing-library/dom-testing-library/commit/771c1901/@testing-library/dom"

it works.

When I add the same resolution to my main branch using jest v26 it still works - my guess is that it is getting v27 of pretty-format but the way it is now imported in this PR is backwards compatible with both v26 and v27.

@eps1lon
Copy link
Member

eps1lon commented Jun 1, 2021

Do you want any more info from me?

A cloneable repro. These error messages do not help me identify the issue. They only show symptoms.

So, if node imports testing-library first, then it will cache v26 of pretty-format for all requests within that process.

That's not how node's resolution algorithm works. It resolves each package relative to each other (https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders)

If I have jest -> pretty-format@27 and testing-library -> pretty-format@26 then testing-library will always get v26 and jest v27. Everything else is either a bug in node (highly unlikely) or a bug in your package manager/bundler or possibly jest itself. Otherwise semantic versioning would be useless since every package would just get a single version that just happens to be loaded first.

I think we're just going to inline pretty-format. We've had two or three issues with it in the past and by inlining pretty-format we can solve #907. It's now clear to me that pretty-format is not suiteable for consumption outside of jest packages.

@MatanBobi
Copy link
Member Author

That's not how node's resolution algorithm works. It resolves each package relative to each other (https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders)

If I have jest -> pretty-format@27 and testing-library -> pretty-format@26 then testing-library will always get v26 and jest v27. Everything else is either a bug in node (highly unlikely) or a bug in your package manager/bundler or possibly jest itself. Otherwise semantic versioning would be useless since every package would just get a single version that just happens to be loaded first.

I was under the same impression you were but then I saw jest-resolve-dependencies package and wasn't sure what's going on..
I also saw this one and thought that maybe jest are doing something in their code but wasn't able to understand and didn't have a lot of time..

@lukeapage
Copy link

You are right from a normal node perspective:

https://nodejs.org/api/modules.html#modules_module_caching_caveats

it sounds like you do not need a repro if you already have plans to fix this. If your stuck and nothing will happen without me making a repro I will attempt to make one.

@eps1lon
Copy link
Member

eps1lon commented Jun 3, 2021

jest might resolve dependencies overly aggressive. I need pretty format v27 for #907 anyway so it'll be fixed in the next major anyway. Which makes it also safer for people still on jest 26

@MatanBobi
Copy link
Member Author

@eps1lon wdym when you say overly aggressive?
I'm closing this one if you say you already need pretty-format 27 as part of a different fix.

@MatanBobi MatanBobi closed this Jun 6, 2021
@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2021

@eps1lon wdym when you say overly aggressive?

It's de-deduplicating packages it shouldn't e.g. v26 and v27 are required by different packages but jest just gives every package a single version.

@MatanBobi
Copy link
Member Author

MatanBobi commented Jun 6, 2021

@eps1lon wdym when you say overly aggressive?

It's de-deduplicating packages it shouldn't e.g. v26 and v27 are required by different packages but jest just gives every package a single version.

This sounds like a bug to me.. Do you know if that's the wanted behavior?

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2021

This sounds like a bug to me

I said "might".

Do you know if that's the wanted behavior?

I wouldn't know

@MatanBobi MatanBobi deleted the chores/support-jest-27-pretty-dom branch June 6, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not compatible with jest 27
3 participants