-
Notifications
You must be signed in to change notification settings - Fork 578
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
RJS-2187: Add progress information to RealmProvider fallback. #6801
Conversation
if (progressValues instanceof Array) { | ||
const sendProgress = () => { | ||
// Uses act as this causes a component state update. | ||
act(() => callback(progressValues[progressIndex])); |
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 wonder if this would be better to be moved into the calling callback instead 🤔 To keep this mock a bit cleaner (and independent of @testing-library/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.
Ah, the calling callback in this case is RealmProvider
so I do not have much control over that callback sadly; I could create the option to let the user define a function wrapper but seems too much for a convenience method
}, | ||
); | ||
|
||
const delayedRealmOpen = jest.spyOn(Realm, "open"); |
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) is this mock restored? Perhaps this side-effect could be moved into the test where you could also call jest.restoreAllMocks() to avoid this state bleeding into other tests.
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 have been running the cleaning but I agree it's not intuitive that this side effect exists. Maybe even extending this jest with like jest.mockRealmOpen()
could be the most consistent with rest of jest mock API to have these mock helpers (and then intuitively jest.restoreAllMocks()
cleans them).
Alternatively we wrap them in a hook which includes a clean.
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.
Seems like extending jest isn't the norm as it is with extending expect
so I guess we either can define a hook with cleanup built in (although then if we have multiple mocks we'd be running the cleanup too much) or use the function as is and just make sure to run the restoreAllMocks
@realm/devdocs
What, How & Why?
Solves #5459
☑️ 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 necessary@realm/devdocs
if documentation changes are needed