-
Notifications
You must be signed in to change notification settings - Fork 348
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
Replace react dom's testing library with ReactTestingLibrary #859
Conversation
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 afraid I don't know the reason why switching to another (heavier) testing library is needed.
This directory contains a vendored version of the melange-testing-library. | ||
|
||
The original repository is https://github.com/melange-community/melange-testing-library |
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.
The PR description mentions
Vendors ReactTestingLibrary (and does some ad-hoc changes)
are the ad-hoc changes valuable for other users of the bindings? in that case, will they be upstreamed?
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.
Not sure, melange-testing-library contains a few binding designs focused on previous versions of BuckleScript where options aren't translated to undefined, or records aren't objects.
I would keep the changes here and evolve this bindings as a playground for the melange-testing-library
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.
works, but we should upstream as they get better!
@@ -54,11 +59,19 @@ format-check: ## Checks if format is correct | |||
.PHONY: install | |||
install: ## Update the package dependencies when new deps are added to dune-project | |||
@opam install . --deps-only --with-test | |||
@npm install | |||
@npm install --force |
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.
Why was force
needed?
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.
Because testinglib and other deps depend on react 18, so npm complains. It is a decent workaround to ensure the installation succeeds
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.
Will write 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.
would we still needed --force
if you commit the lockfile from manual install and run npm ci
here?
…esting-libraries * '19' of github.com:/reasonml/reason-react: Add deprecations on ReactDOMTestUtils Add uri comment back on action Update src/React.re Update src/React.re Update src/React.re Snapshot with lower {}
f7d55fe
to
7aaef3e
Compare
…on-react into replace-testing-libraries * 'replace-testing-libraries' of github.com:/reasonml/reason-react: Bind React.act and React.actAsync
…lid-prop * '19' of github.com:/reasonml/reason-react: Replace react dom's testing library with ReactTestingLibrary (#859)
…lid-prop * '19' of github.com:/reasonml/reason-react: Replace react dom's testing library with ReactTestingLibrary (#859)
…ises-as-annotations * '19' of github.com:/reasonml/reason-react: Enable ref as valid prop (#862) Replace react dom's testing library with ReactTestingLibrary (#859) Add deprecations on ReactDOMTestUtils Add uri comment back on action Update src/React.re Update src/React.re Update src/React.re Snapshot with lower {}
Depends on #846
demo
folder to ensure React works manually, loads, etc.