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

Make the react-native entry point Jest compatible #6012

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Make the react-native entry point Jest compatible #6012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* None

### Fixed
* <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-js/issues/????), since v?.?.?)
* Fix Jest issues when testing against Realm ([#6003](https://github.com/realm/realm-js/issues/6003))
* None

### Compatibility
Expand Down
30 changes: 0 additions & 30 deletions packages/realm-react/custom-resolver.cjs
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions packages/realm-react/jest.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"verbose": true,
"preset": "react-native",
"resolver": "./custom-resolver.cjs",
"roots": [
"<rootDir>/src"
],
Expand All @@ -13,4 +12,4 @@
},
"testRegex": "(/__tests__/.*.(test|spec)).(jsx?|tsx?|js?|ts?)$",
"testTimeout": 30000
}
}
17 changes: 16 additions & 1 deletion packages/realm/index.react-native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,19 @@
/* eslint-disable @typescript-eslint/no-var-requires -- We're exporting using CJS assignment */
/* eslint-env commonjs */

module.exports = require("./dist/bundle.react-native").Realm;
// eslint-disable-next-line no-undef -- In React Native, process is not defined, but in Jest it is
const isJest = process?.env?.JEST_WORKER_ID !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit sad that we need to do this 😞 It would be better if we could find a way to keep this pure from runtime checks of the environment ... historically we've been bitten by the brittleness of these many times before.

Copy link
Member

Choose a reason for hiding this comment

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

Also - I'm surprised you're not getting an Uncaught ReferenceError: process is not defined when running on React Native 🤔

Copy link
Contributor Author

@takameyer takameyer Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

We can't get around the react-native preset for jest, which is basically ensuring the react-native entry point is being loaded. I did a lot of digging around this and this was the best solution I could find. At least it's very dependent on Jest and has very little impact on the experience.

Copy link
Contributor Author

@takameyer takameyer Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

'm surprised you're not getting an Uncaught ReferenceError: process is not defined when running on React Native 🤔

I need to refine the comment. process is actually defined in react-native, but for some reason it is not typed.

Copy link
Member

@kraenhansen kraenhansen Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

which is basically ensuring the react-native entry point is being loaded

As per that link, it looks to me that we could probably export the Node.js bundle via the require condition (since it's giving priority to require over react-native and the opposite is true for the defaults of the metro bundler) and avoid the runtime check?

https://github.com/facebook/react-native/blob/1ba7bc0eb1099b64601e0037f3ef717fcd1302e3/packages/react-native/jest/react-native-env.js#L15C9-L15C9

Copy link
Contributor Author

@takameyer takameyer Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

How do we set the require condition for our users?
The linked code is ran by adding preset: "react-native" to the jest config, which is automatically done when initializing a React Native project. Jest isn't looking for this automatically in imported packages, as far as I can tell.
Jest also doesn't allow for multiple presets

Copy link
Member

@kraenhansen kraenhansen Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

Add this line

      "require": "./index.node.js",

after

realm-js/packages/realm/package.json

Line 31 in 0b9dd8b

"node": "./index.node.js",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh boy...that works!


let entryPoint;

if (isJest) {
// Define a require function that will load the node bundle
// otherwise, metro will preemptively load the node bundle
const nodeRequire = require;
// Jest is running, use the node bundle
entryPoint = nodeRequire("./dist/bundle.node");
} else {
entryPoint = require("./dist/bundle.react-native");
}

module.exports = entryPoint.Realm;
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest simply inlining this assignment. No need to store it in entryPoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I was doing a lot of dancing around how metro bundled this, to ensure it didn't pre-load the node entry point. Setting require to a variable was the solution.