-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
feat(jest): move Jest config to use a custom react-native Jest env #34971
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/** | ||
* Copyright (c) Meta Platforms, Inc. and affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @format | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const NodeEnv = require('jest-environment-node').TestEnvironment; | ||
|
||
module.exports = class ReactNativeEnv extends NodeEnv { | ||
// this is where we'll add our custom logic | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see an RFC for Might make sense to override There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess ideally this env would get the option from metro/metro config somehow instead of defining its own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah I'd rather have this one be compatible, then do things on top. Maybe post a comment in the RFC itself btw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which of them? the one I opened about the env or the one for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the export one, it will be a fairly important one for a couple reasons so if we have a reminder of this topic in there it's more likely we'll keep it in mind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool, added some comments now 👍 |
||
}; |
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.
Should add a dependency on
jest-environment-node
- otherwise you rely on hoisting.When you stop extending the existing env, that dep can of course be removed.
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.
sounds good - originally I added it to the repo-config package.json, but since it didn't change the
yarn.lock
I assumed it was part of the dependencies that jest itself was bringing along. Will modify PR accordingly 👍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.
that's correct, but it would then rely on hoisting 🙂
It needs to be part of the root package.json btw - same level as the preset lives on. Which might introduce issues since there's no
jest
there... Possibly an optional peer dep makes more sense than a direct dependency? Again, will be removed when you stop extending.(it might make sense to just copy the entire content of
jest-environment-node
now instead of empty extend? That way, no dep)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.
yeah the thing is that the repo-config/package.json is this really weird thing (the plan is to remove it)... goes back to the monorepo conversation here react-native-community/discussions-and-proposals#480
I'm going to guess this dep should live alongside Jest, and that means that I'll have to add it to the repo-config one and then modify the magic script that does some stuff when branch cutting to ensure that on released versions it gets moved along (you can see what I mean here c012726#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519)
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.
That won't hit same issue as #31668 (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.
Very true 😅 separate package for the preset entirely is probably the least wrong approach (longer term - short term this should work a treat of course)
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.
right, almost forgot - added a "next steps" section to the PR body with all the things that should be done as follow ups :)
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 feel like I mentioned it but I can't find it so might've been a fever dream.
An option to a dep is optional peer dep. Sorta depends on timeline for 0.71 - seems suboptimal for
yarn add react-native
to bundle a jest env.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.
Fair point. Correct me if I'm wrong - that would also mean adding
jest-environment-node
to the template'sdevDependencies
explicitly (alongsidejest
)? I don't think peer dependencies are guaranteed to be hoisted if they're only provided transitively (viajest
in this case), I think they're expected to be specified by the root project.We'd have to introduce
peerDependenciesMeta
also, sincereact-native
doesn't currently have any optional peer deps.I'm ok with the current
dependencies
approach for a few reasons;package.json
,jest-environment-node
is small,jest-environment-node
@cortinico, @hramos , @motiz88 - want to weigh in here?
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.
(for context: we are aiming to cut 0.71 over the next couple weeks)