-
Notifications
You must be signed in to change notification settings - Fork 452
Update fetch-mock-jest to @fetch-mock/jest #5488
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
Conversation
| fetchMock.mockGlobal(); | ||
| global.fetchMock = fetchMock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was afraid that this could be an issue when running tests in parallel, but I couldn't make it fail. I think that nowadays jest (or is it the jsdom environment, or just new versions of jsdom) isolates the globals better than it used to.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5488 +/- ##
==========================================
- Coverage 86.15% 86.11% -0.04%
==========================================
Files 307 308 +1
Lines 29637 29652 +15
Branches 7999 8001 +2
==========================================
+ Hits 25533 25536 +3
- Misses 3516 3526 +10
- Partials 588 590 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // in jest apparently. | ||
| // Still let's double check that they're from the global scope as expected, so | ||
| // that this can be removed once it's implemented. | ||
| if ('TextDecoder' in this.global) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully sure this condition works, TBH... I don't know if this.global contains all the globals, or contains only the new things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.
Nice, thanks for checking!
I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.
Ah, do we have TextDecoder in v30?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextDecoder I don't know, but Request, Response, fetch are there (but not ReadableStream surprisingly!)
| import { TestEnvironment } from 'jest-environment-jsdom'; | ||
| import { TextDecoder, TextEncoder } from 'util'; | ||
|
|
||
| export default class CustomTestEnvironment extends TestEnvironment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from https://www.wheresrhys.co.uk/fetch-mock/docs/wrappers/jest#jsdom-campatibility, adapted for our use case.
canova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But wow, that's surprisingly a lot of changes. I always get annoyed when a utility/testing package has this big of a breaking change (insert "old man yells at cloud" meme). To be fair, technically it's a different package, but still... At least I'm happy that I didn't have to deal with this, so thank you! 😄
I have some questions but they are mostly so I understand better, not blocking.
| ], | ||
| "transformIgnorePatterns": [ | ||
| "/node_modules/(?!(query-string|decode-uri-component|split-on-first|filter-obj)/)" | ||
| "/node_modules/(?!(query-string|decode-uri-component|split-on-first|filter-obj|@fetch-mock/jest|fetch-mock)/)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I can't seem to remember this. Why do we need this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because our current configuration doesn't deal with esm import and export when running in jest (I think it should be possible to make it work easily nowadays...).
This configuration property configures jest to not run babel on some files, by default this is all of node_modules. We override this line to run babel on some npm packages that are using export/import.
| // in jest apparently. | ||
| // Still let's double check that they're from the global scope as expected, so | ||
| // that this can be removed once it's implemented. | ||
| if ('TextDecoder' in this.global) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.
Nice, thanks for checking!
I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.
Ah, do we have TextDecoder in v30?
| const isObject = (subject) => | ||
| Object.prototype.toString.call(subject) === '[object Object]'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we need a function like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that the object we get inherits from the Object object in another js environment.
Also the previous code didn't work for an object without a prototype (such as when created with Object.create(null)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of wonder if this might be a bug in jest though.
I've seen this in jest's node environment => https://github.com/jestjs/jest/blob/42c8e7de64d1a3f8ee9b7f3b309332ac8f88e8b1/packages/jest-environment-node/src/index.ts#L157-L161
but it's not in the jsdom environment.
This might be a red herring though.
3f5f7e5 to
7aa6c5f
Compare
Changes: [Nazım Can Altınova] Fix issues related to the track borders (#5484) [Nazım Can Altınova] Remove the active tab and origins views (#5483) [Julien Wajsberg] Automatically request reviews for dependency updates (#5490) [Julien Wajsberg] Update fetch-mock-jest to @fetch-mock/jest (#5488) [Steve Fink] Document marker filter syntax (#5493) [Nazım Can Altınova] Update all of jest 29.7.0 → 30.0.0 (major) (#5495) [Nazım Can Altınova] Order global tracks by activity and select the most active non-parent process by default (#5491) [Paul Adenot] Allow searching for HTTP response status in marker views (#5504) [Nazım Can Altınova] Expose a `totalMarkerDuration` function in console (#5507) [Nazım Can Altınova] 🔃 Sync: l10n -> main (July 11, 2025) (#5510) And thanks to our localizers: el: Jim Spentzos kab: ZiriSut tr: Grk tr: Selim Şumlu uk: Artem Polivanchuk
This PR updates the obsolete fetch-mock-jest to its new version @fetch-mock/jest.
Unfortunately this comes with lots of syntax changes. In particular, previously
fetchwas both the mock and the configuration object, but now they're 2 different objects.There's a useful migration page: https://www.wheresrhys.co.uk/fetch-mock/docs/Usage/upgrade-guide
The fact this patch also removes node-fetch brought other problems, I believe due to the interaction with jsdom or jest's vm mechanism, with different cross-realm constructors, so I had to minimally change some app code as well.
Also due to jsdom's not implementing fetch and other fetch-related objects that are now available in nodejs by default (even if not exactly the same), I had to supply a custom environment.
I tried to split the PR by logical commits.
Tell me what you think!