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.

Sign up for GitHub

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

Jump to bottom

feat(jest): move Jest config to use a custom react-native Jest env #34971

Closed
wants to merge 3 commits into from
+18 −1
Closed

feat(jest): move Jest config to use a custom react-native Jest env #34971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de24efa
add a custom react-native jest env
kelset Oct 13, 2022
399651a
prettier
kelset Oct 13, 2022
7026c14
adding to the deps in root package.json
kelset Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion jest-preset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ module.exports = {
'node_modules/(?!((jest-)?react-native|@react-native(-community)?)/)',
],
setupFiles: [require.resolve('./jest/setup.js')],
testEnvironment: 'node',
testEnvironment: require.resolve('./jest/react-native-env.js'),
};
16 changes: 16 additions & 0 deletions jest/react-native-env.js
View file
Open in desktop
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;
Copy link
Contributor

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.

Copy link
Collaborator Author

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 👍

Copy link
Contributor

@SimenB SimenB Oct 14, 2022 •
edited

Choose a reason for hiding this comment

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

since it didn't change the yarn.lock I assumed it was part of the dependencies that jest itself was bringing along

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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)

Copy link
Contributor

@SimenB SimenB Oct 14, 2022 •
edited

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)?

Copy link
Contributor

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)

Copy link
Collaborator Author

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 :)

Copy link
Contributor

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.

Copy link
Contributor

@robhogan robhogan Oct 14, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Fair point. Correct me if I'm wrong - that would also mean adding jest-environment-node to the template's devDependencies explicitly (alongside jest)? I don't think peer dependencies are guaranteed to be hoisted if they're only provided transitively (via jest in this case), I think they're expected to be specified by the root project.

We'd have to introduce peerDependenciesMeta also, since react-native doesn't currently have any optional peer deps.

I'm ok with the current dependencies approach for a few reasons;

  • simplicity, especially re upgrade paths for userland package.json,
  • jest-environment-node is small,
  • we're moving towards a monorepo,
  • we plan to drop the dependency on jest-environment-node

@cortinico, @hramos , @motiz88 - want to weigh in here?

Copy link
Collaborator Author

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)


module.exports = class ReactNativeEnv extends NodeEnv {
// this is where we'll add our custom logic
Copy link
Contributor

Choose a reason for hiding this comment

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

I see an RFC for exports is up now, with the expected react-native condition: https://github.com/react-native-community/discussions-and-proposals/pull/534/files#diff-40781647168e437e711b426935003e99ce4bddb4c1a5f1ebd61b0218d2b45670R294

Might make sense to override exportConditions right away? Or do you want this PR to be 100% compatible, then make changes to the env iteratively?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100% compatible, then make changes to the env iteratively?

yeah I'd rather have this one be compatible, then do things on top. Maybe post a comment in the RFC itself btw?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 exports?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, added some comments now 👍

};
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
"base64-js": "^1.1.2",
"event-target-shim": "^5.0.1",
"invariant": "^2.2.4",
"jest-environment-node": "^29.0.3",
"jsc-android": "^250230.2.1",
"memoize-one": "^5.0.0",
"metro-react-native-babel-transformer": "0.72.3",
Expand Down