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

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

Closed
wants to merge 3 commits into from

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Oct 13, 2022

Summary

This PR is the follow up to the conversation started here by @SimenB: react-native-community/discussions-and-proposals#509

Basically, we want to move RN to use its own custom environment so that we can tweak it going forward - this PR in fact only sets up the groundwork for that; @robhogan mentioned that with this in place, Meta engineers can

iterate on it (with jest-environment-node as a starting point) against our internal product tests

This is also connected to Rob's work to bring Jest 29 into the codebase #34724 and my "mirror" PR to bring template in main up to the same version (#34972)


Next steps

This PR opens the door for 3 follow up steps:

  • actually customize the custom jest env
  • remove the need of jest-environment-node
  • publish the env as its own package @react-native/jest-env so that it can be consumed more easily from other tools and developers

Changelog

[General] [Changed] - move Jest config to use a custom react-native Jest env

Test Plan

Tested that yarn test in main works fine after the changes; CI and Meta's internal CI will also serve the purpose of verifying that it works (but there's no reason not to since it's still pretty much just relying on node).

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner labels Oct 13, 2022
@kelset kelset requested a review from robhogan October 13, 2022 16:49
@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 8422557
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,776,717 -714
android hermes armeabi-v7a 7,177,493 -929
android hermes x86 8,089,617 -808
android hermes x86_64 8,061,291 -829
android jsc arm64-v8a 9,636,692 -189
android jsc armeabi-v7a 8,400,781 -387
android jsc x86 9,585,781 -277
android jsc x86_64 10,178,934 -292

Base commit: 8422557
Branch: main

Copy link
Contributor

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

👍


'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
Contributor 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

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
Contributor 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

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
Contributor 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

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

@kelset kelset requested a review from hramos as a code owner October 14, 2022 10:13
@github-actions
Copy link

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 7026c14

@facebook-github-bot
Copy link
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

const NodeEnv = require('jest-environment-node').TestEnvironment;

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
Contributor 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
Contributor 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 👍

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @kelset in cb2dcd3.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 17, 2022
@kelset kelset deleted the kelset/add-jest-custom-env branch October 18, 2022 09:17
facebook-github-bot pushed a commit that referenced this pull request Oct 18, 2022
Summary:
This PR is the follow up of #34724 for the template; this way, we'll have version alignment and RN71 will use this.

This wants to be merged along #34971

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Changed] - upgrade Jest in template to 29

Pull Request resolved: #34972

Test Plan: Generated a test project via `yarn test-e2e-local -t RNTestProject` and then run `yarn test` in that project to verify that there's no regression. ✅

Reviewed By: huntie

Differential Revision: D40379963

Pulled By: robhogan

fbshipit-source-id: 618207ed6bf7921d13f0b6a9220dc52c9cf78b14
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…acebook#34971)

Summary:
This PR is the follow up to the conversation started here by SimenB: react-native-community/discussions-and-proposals#509

Basically, we want to move RN to use its own custom environment so that we can tweak it going forward - this PR in fact only sets up the groundwork for that; robhogan mentioned that with this in place, Meta engineers can
> iterate on it (with jest-environment-node as a starting point) against our internal product tests

This is also connected to Rob's work to bring Jest 29 into the codebase facebook#34724 and my "mirror" PR to bring template in main up to the same version (facebook#34972)

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Changed] - move Jest config to use a custom react-native Jest env

Pull Request resolved: facebook#34971

Test Plan: Tested that `yarn test` in main works fine after the changes; CI and Meta's internal CI will also serve the purpose of verifying that it works (but there's no reason not to since it's still pretty much just relying on `node`).

Reviewed By: huntie

Differential Revision: D40379760

Pulled By: robhogan

fbshipit-source-id: 2c6d0bc86d337fda9befce0799bda2f56cc4466c
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This PR is the follow up of facebook#34724 for the template; this way, we'll have version alignment and RN71 will use this.

This wants to be merged along facebook#34971

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Changed] - upgrade Jest in template to 29

Pull Request resolved: facebook#34972

Test Plan: Generated a test project via `yarn test-e2e-local -t RNTestProject` and then run `yarn test` in that project to verify that there's no regression. ✅

Reviewed By: huntie

Differential Revision: D40379963

Pulled By: robhogan

fbshipit-source-id: 618207ed6bf7921d13f0b6a9220dc52c9cf78b14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants