-
Notifications
You must be signed in to change notification settings - Fork 987
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
Upgrade re-frame to latest #19931
Upgrade re-frame to latest #19931
Conversation
Jenkins BuildsClick to see older builds (20)
|
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.
Interesting, I was wondering about reusing the subscriptions logic inside events yesterday :D
c1285fa
to
c756b4c
Compare
Hi @ilmotta ! Thanks for your PR! |
No @mariia-skrypnyk, it should be QAed if possible. This library is a fundamental part of our app. We shouldn't see any impact. I did my fair share of double-checks in this PR, but you'd be able to do a better job for sure. Edit: smoke tests should probably be enough. Upgrading re-frame always involves some kind of risk for us, and that's why I'm trying to do it now, because later on it will only get riskier as we approach deadlines. But seriously, the risk of bugs is very low, but we never know right? |
83% of end-end tests have passed
Failed tests (7)Click to expandClass TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (43)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
c756b4c
to
857bec6
Compare
@ilmotta sure! Your PR will be QAed if needed. |
857bec6
to
f9dd614
Compare
f9dd614
to
0eb80e0
Compare
85% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
Hi @ilmotta ! Smoke test done, all looks good on both platforms👌! Failed e2e are not related and you can merge your changes 🙌! |
Thank you @mariia-skrypnyk. Will merge later today. Next time I'll remember to add the label correctly :) |
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 this PR @ilmotta !
0eb80e0
to
42009a8
Compare
42009a8
to
7e8d915
Compare
Summary
A quick win for us. Upgrade re-frame to latest, from
v1.3.0
(released on 2022-08-27) to latestv1.4.3
(released on 2024-01-25).Nothing outstanding, but this upgrade might entice your curiosity for the following reasons:
re-frame.alpha
namespace, for testing proposed features (see flows and polymorphic subscriptions). If you haven't read before, you've been warned, these links can be a rabbit hole.dispatch-sync
now emits a:sync
trace to indicate when it has finished. @flexsurfer you might care about this for re-frisk.v1.2.0
.v1.4.0
, but they don't affect us because we don't use interceptorspath
andunwrap
.Note
With the introduction of
re-frame.alpha
, it is my expectation that, by upgrading re-frame, we make it easier for core devs to experiment with new ideas to solve old problems 😈Areas that may be impacted
Nothing in theory.
Steps to test
I ran smoke tests and the app is behaving the same, no warnings, which is to be expected, given the few breaking changes reported in re-frame's changelog don't affect us.
status: ready