-
Notifications
You must be signed in to change notification settings - Fork 563
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
Make the react-native entry point Jest compatible #6012
Conversation
7134e8f
to
6e112c2
Compare
* Remove the need to create a custom resolver in jest code testing realm * This ensures there are no breaking changes in current users tests
6e112c2
to
8d17cbd
Compare
entryPoint = require("./dist/bundle.react-native"); | ||
} | ||
|
||
module.exports = entryPoint.Realm; |
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'd suggest simply inlining this assignment. No need to store it in entryPoint
.
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.
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.
@@ -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; |
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'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.
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.
Also - I'm surprised you're not getting an Uncaught ReferenceError: process is not defined
when running on React Native 🤔
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.
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.
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.
'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.
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.
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?
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.
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
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.
Add this line
"require": "./index.node.js",
after
realm-js/packages/realm/package.json
Line 31 in 0b9dd8b
"node": "./index.node.js", |
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.
Oh boy...that works!
What, How & Why?
This closes #6003
☑️ ToDos
Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessaryIf this PR adds or changes public API's: