-
-
Notifications
You must be signed in to change notification settings - Fork 326
chore: change relay test #23390
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
chore: change relay test #23390
Conversation
bd05de6 to
be44f4f
Compare
be44f4f to
fd6b32d
Compare
fd6b32d to
9ea2a73
Compare
| ); | ||
|
|
||
| if (!originalIsSuiteSyncEnabled) { | ||
| dispatch((_, getState) => initSuiteSyncNative({ getState })); |
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 was forgotten, there is nothing to init anymore
| "@suite-native/sentry": "workspace:*", | ||
| "@suite-native/settings": "workspace:*", | ||
| "@suite-native/storage": "workspace:*", | ||
| "@suite-native/suite-sync": "workspace:*", |
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.
βοΈ important, this shall not be needed at all, all shall use suite-sync from common and call everything against interface
| dispatch: Dispatch; | ||
| }; | ||
|
|
||
| export const createSuiteSyncCompositionRoot = ( |
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 is important concept.
We can have modules (suite-sync) that can (shall?) handle their composition in isolatation. So the main app is not polluted by 1000 servicies from it.
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.
cc @vojtatranta
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.
Yes , logicaly "vztyΔna plocha" should be minimal - the interaction between this "suit sync module" and the rest of the app.
Logically then, anything that doesn't need to be called from outside shouldn't be there. However, it doesn't solve our problem with that Big Extradependencies that needs to know about these.
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.
Yea, it does not solve problem with Big ExtraDependencies, because they are the problem. They are not composition root, they are the Service-Locator.
They create inherent circular dependency.
If you have object that contain EVERYTHING and you want to use it as type SOMEWHERE, that SOMEWHERE is already referenced in EVERTHING, because it is fucking EVERYHING π€£
9ea2a73 to
8bab759
Compare
c7220c2 to
376f13c
Compare
376f13c to
fd3b9b1
Compare
8bab759 to
37e0ed8
Compare
fd3b9b1 to
1299cb2
Compare
packages/suite/src/views/settings/SettingsDebug/SuiteSyncSettings.tsx
Outdated
Show resolved
Hide resolved
packages/suite/src/views/settings/SettingsDebug/SuiteSyncSettings.tsx
Outdated
Show resolved
Hide resolved
37e0ed8 to
f228d8b
Compare
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| const dispatch = useDispatch(); | ||
| const { suiteSync } = useServices(); |
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 is how to use dependencies/servicies in component. This can be improved, this is very simple solution.
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.
Ideally components shall have defined dependencies as well, but it is probably not very doable in React
f228d8b to
2f550d0
Compare
c5a495e to
6f98ce0
Compare
| } | ||
|
|
||
| return acc; | ||
| }, [] as SuiteSyncOwner[]); |
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 would be appropriate as a memoizedSelector
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.
fixed
9126e97 to
4659d46
Compare
6f98ce0 to
556e676
Compare
|
β
Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
556e676 to
037d37c
Compare
|
β
Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
719fb1f to
037d37c
Compare
feat(suite): add option to pass extra dependencies factory to store and as extra thunk parameter and use it freely also from react context chore: change Relay Test # Conflicts: # suite-native/suite-sync/package.json # suite/suite-sync/package.json # yarn.lock
037d37c to
3562129
Compare
Lemonexe
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.
LGTM
Adds a test for changeRelayUrl + some refactoring around it.
This is followup on #23401
ππ₯οΈ Suite native android test results: View in Currents
ππ₯οΈ Suite web test results: View in Currents
ππ₯οΈ Suite desktop test results: View in Currents