-
Notifications
You must be signed in to change notification settings - Fork 2
FED-3659 Add Dart bindings for React act #86
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
Conversation
greglittlefield-wf
left a comment
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.
Just a couple additional test cases I thought would be good to add, otherwise this looks great!
|
|
||
| await rtl.act(asyncCallback); | ||
| expect(useEffectCalls, 2); | ||
| }); |
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.
Could we also add some tests that verify that any errors in the callback successfully propagate, both for sync and async callbacks?
That way
a) we'll verify that the errors aren't swallowed and
b) we'll know whether anything weird happens to Dart exceptions in the JS interop that might require throwErrorFromJS or something).
Could do something like this (I stumbled across that when looking through code trying to remember the name of throwErrorFromJS):
react_testing_library/test/unit/dom/wait_for_test.dart
Lines 122 to 123 in 8288d6f
| expect(() => rtl.waitFor(() => throw ExceptionForTesting(), container: view.container), | |
| throwsA(isA<ExceptionForTesting>())); |
greglittlefield-wf
left a comment
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.
Code LGTM, but I just remembered 🤦, this repo is set up to auto-release/publish now on merge.
Would you mind updating the changelog for 3.1.0 with this addition? And if you wouldn't mind, filling in 3.0.3/3.0.4 too while you're in there? 😅
greglittlefield-wf
left a comment
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.
Thanks for updating the changelog!
+10
@Workiva/release-management-p
rmconsole-wf
left a comment
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.
+1 from RM
Motivation
We should expose the
actAPI from React (https://react.dev/reference/react/act)While most cases are handled by RTL, whose APIs such as
UserEventandfireEventare wrapped in act:...there are some cases, like testing component imperative API calls, or dispatching actions directly, in which
actmay be a good fit.Changes
actin dartRelease Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: