-
Notifications
You must be signed in to change notification settings - Fork 68
FED-3283: React 18 js files #414
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
NOTES: Context’s calculateChangedBits was removed or altered… not sure which.
Conflicts: build.yaml example/test/unmount_test.dart lib/react_client/react_interop.dart lib/react_test_utils.dart lib/src/react_client/dart2_interop_workaround_bindings.dart lib/src/react_client/private_utils.dart
Remove/repurpose console.warning wrapper that's no longer needed (see previous commit).
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.
This looks amazing! Just one tiny comment on the README.
Didn't get a chance to QA, but I can do that next week!
mockito:mockBuilder: | ||
# Scope only to files declaring mocks, for performance. | ||
generate_for: | ||
- test/mockito.dart |
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.
Looks like this got left behind from when we switched to mocktail; thanks for cleaning this up!
@@ -57,7 +57,7 @@ void validateJsApi() { | |||
registerComponent2(() => _DummyComponent2()); | |||
_isJsApiValid = true; |
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.
#idea One thing I was thinking of that would be helpful for QAing this PR, but also would be helpful in consumer repos. While we have this dual-version setup, it might be nice to log the version of React being used so we can easily validate what tests are running with
Unfortunately just a console.log
in the JS bundle won't make its way to the Dart test output, and it needs to be a print
statement triggered from a test Zone.
I was just thinking, that most usages of this package indirectly call this function pretty early on, and this might actually be a good place to stick it... For example:
if (inReactDevMode) print('React version: ${React.version}');
_isJsApiValid = true;
That might need a little more discovery and could be done in a follow-up though. Hmm yeah and thinking about it, one downside is it would get logged for every test file that gets run, which might not be ideal...
Yeahh maybe we do that as a follow-up haha
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.
- Changes look good
- React-redux version matches between yarn.lock/pnpm.lock - upgrading this requires a separate effort
- No changes to existing (React 17) JS files
- JS assets and lockfile are up to date
- Dual version-testing setup
- Verified tests run with the correct versions
- Verified test presets work and fail correctly when the wrong version is being used
- over_react CI passes when consuming React 18 assets (along with RTL dual-support branch): Test consume react-dart dual support (force React 18) over_react#977
- Examples look good
+10
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
@Workiva/release-management-p |
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
Upgraded React to 18 (most of that work was done in React 18.2.0 #365
Migrate from yarn to pnpm
Migrate from webpack to Vite
Update
js_src
files to use module syntax and do a bit of general cleanup.Generate
lib/js/react.dev.js
andlib/js/react.min.js
(with sourcemap for min version)Remove corejs polyfilling (wdesk_login is the only place that needs to support ie11 and it has its own polyfilling)
Leaving the
baseURI
polyfill in, just in case.Update Dart test setup to use
test_html_builder
to make it easier to swap template being used.Update CI matric to run tests against both react 17 and react 18 assets.
Added explicit react 18 bundle tests
Added tags
react-17
(wont run when using react 18) andreact-18
(wont run when using react 17)Added test to explicitly check the version of react being used in tests.