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: update Jest #7220

Merged
merged 1 commit into from
Aug 29, 2023
Merged

chore: update Jest #7220

merged 1 commit into from
Aug 29, 2023

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented Aug 22, 2023

Without this update #7217 & #7219 will keep failing


CC/ @SimenB to make sure we're doing what we're supposed to do

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2023

⚠️ No Changeset found

Latest commit: 237aa7c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@SimenB
Copy link

SimenB commented Aug 22, 2023

Nothing here jumps out to me. A bit odd removing require.resolve fixes things, but shouldn't hurt.


Any particular reason why you don't go to v29? I'd be happy to help upgrading (I actually sent a PR at work 6 hours ago introducing react-router into the code base 😅 So happy to invest some time into this (and dependant) project)

@@ -15,9 +15,12 @@ module.exports = {
transform: {
"\\.[jt]sx?$": require.resolve("./transform"),
},
transformIgnorePatterns: [
"node_modules/(?!(@remix-run/web-(blob|fetch|file|form-data|stream)|@web3-storage/multipart-parser)/)",
Copy link
Member Author

@MichaelDeBoey MichaelDeBoey Aug 22, 2023

Choose a reason for hiding this comment

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

We will be able to remove the @remix-run/web-* dependencies here once remix-run/web-std-io#43 is merged

CC/ @SimenB Tagging you to make sure I'm getting this right

@MichaelDeBoey
Copy link
Member Author

@SimenB My plan was to first upgrade to v28 and make that work, but I can go straight to v29 as well if you think that would make it easier for you

@SimenB
Copy link

SimenB commented Aug 22, 2023

I'm somehow surprised there's a difference. 😅 Breaking changes 28->29 (beyond snapshot format changes, which are configurable - and node versions) were chosen as they were negligible.

@MichaelDeBoey
Copy link
Member Author

@SimenB I just didn't look into upgrading to v29 yet 🤷‍♂️
So if you say there's no real difference, I'll just go ahead and upgrade to v29 👍

@SimenB
Copy link

SimenB commented Aug 22, 2023

Ah! Fair enough 🙂 happy to take a look if it proves troublesome - feel free to tag me 👍

@MichaelDeBoey MichaelDeBoey force-pushed the update-jest branch 3 times, most recently from 0535003 to eeae1d9 Compare August 22, 2023 20:30
@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Aug 22, 2023

@SimenB we're now getting the following warning

● Validation Warning:

Unknown option "watchPlugins" with value ["jest-watch-select-projects", "jest-watch-typeahead/filename", "jest-watch-typeahead/testname"] was found.
This is probably a typing mistake. Fixing it will remove this message.

Configuration Documentation:
https://jestjs.io/docs/configuration

Looking at the docs, I don't see what we're doing wrong tbh as watchPlugins is just there 🤔
https://jestjs.io/docs/configuration#watchplugins-arraystring--string-object

Next to that we're still getting the same read-only errors as we already had in v28

TypeError: Cannot assign to read only property 'atob' of object '[object global]'

Together with the non-constructor errors (which are probably because of the globals not being set)

TypeError: Response is not a constructor

@MichaelDeBoey MichaelDeBoey force-pushed the update-jest branch 5 times, most recently from c6ad996 to 61bd74f Compare August 22, 2023 23:45
@SimenB
Copy link

SimenB commented Aug 23, 2023

Unknown option "watchPlugins"

sounds like jestjs/jest#13576 🙁 should probably dig deeper into it and figure out the root cause. it's just a warning tho

Next to that we're still getting the same read-only errors as we already had in v28

probably jestjs/jest#12642 (currently https://github.com/jestjs/jest/blob/642267f6848869a64861c3176eb5a6d5980639c6/packages/jest-environment-node/src/index.ts#L79-L128)

both atob and btoa are already globals in Node since v16, so I don't think you need to polyfill them at all? it should at least be done conditionally

@MichaelDeBoey
Copy link
Member Author

Agreed - I'd manually polyfill fetch in the jest setup file

@SimenB That's the thing, that's what we're doing with calling installGlobals, which is then adding the polyfills from @remix-run/web-*, which is then using the built-in browser ones (because jsdom environment is using the browser export) 🙈😅

testEnvironment: "jsdom",


export function installGlobals() {
global.atob = atob;
global.btoa = btoa;
global.Blob = NodeBlob;
global.File = NodeFile;
global.Headers = NodeHeaders as typeof Headers;
global.Request = NodeRequest as typeof Request;
global.Response = NodeResponse as unknown as typeof Response;
global.fetch = nodeFetch as typeof fetch;
global.FormData = NodeFormData;
global.ReadableStream = NodeReadableStream;
global.WritableStream = NodeWritableStream;
}

@SimenB
Copy link

SimenB commented Aug 25, 2023

If you want the node polyfilling and not the browser ones, you can set up a module mapper using require.resolve. Or override exportConditions: https://jestjs.io/docs/configuration#testenvironmentoptions-object

@MichaelDeBoey
Copy link
Member Author

@SimenB I'm personally not a fan of overriding the exportConditions I think, as that would mean all dependencies will use the node export instead of the browser one, right?
If so, this could hide some other bugs I suppose 🤔

So I guess moduleNameMapper is the only solution here?

@SimenB
Copy link

SimenB commented Aug 25, 2023

least hacky is probably just

if (typeof window.fetch === 'undefined') {
  require('whatwg-fetch');
}

in your jest setup or something like that. I'd say that's the least likely to hide actual bugs

@MichaelDeBoey MichaelDeBoey force-pushed the update-jest branch 3 times, most recently from a3a09dd to 18ed744 Compare August 26, 2023 20:41
jest.config.js Outdated Show resolved Hide resolved
jest/jest.config.shared.js Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey merged commit 3e36c8d into remix-run:dev Aug 29, 2023
9 checks passed
@MichaelDeBoey MichaelDeBoey deleted the update-jest branch August 29, 2023 15:35
@SimenB
Copy link

SimenB commented Aug 30, 2023

🎉

@@ -23,10 +23,13 @@ module.exports = {
"packages/remix-server-runtime",
"packages/remix-testing",
],
reporters:
process.env.GITHUB_ACTIONS === null
Copy link

@SimenB SimenB Aug 30, 2023

Choose a reason for hiding this comment

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

should be == - env variables that aren't set are undefined, not null.

 $ node -p 'process.env.SOMETHING_RANDOM === null'
false

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in 517e40b

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-e3d5b17-20230916 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed dependencies Pull requests that update a dependency file package:eslint-config package:testing renderer:react v2 Issues related to v2 apis
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

4 participants