[perf] network module #8392
Merged
[perf] network module #8392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (21)
|
rasom
force-pushed
the
network-module
branch
2 times, most recently
from
9f933de
to
b994d2a
Compare
✔️ status-react/prs/ios/PR-8392#3 🔹 ~15 min 🔹 b994d2a 🔹 📦 ios package |
mandrigin
approved these changes
Jun 11, 2019
flexsurfer
reviewed
Jun 11, 2019
| @@ -0,0 +1,20 @@ | |||
| (ns status-im.accounts.model | |||
There was a problem hiding this comment.
i believe usually we use core name
There was a problem hiding this comment.
not really, the code from db ns was moved there in order to solve circular dependency. It is "must contain getter and setter functions used by fx producing functions and subscriptions" from https://github.com/status-im/status-react/blob/develop/doc/codebase-structure-and-guidelines.md , which has no real reason to be in the same ns with specs. I can rename it differently if you have some ideas.
| [re-frame.core :as re-frame] | ||
| [status-im.utils.utils :as utils])) | ||
|
|
||
| (handlers/register-handler-fx |
There was a problem hiding this comment.
with new fx/defn we don't need to separate events anymore, just use {:events []} in fx/defn and you can move all these handlers to core
There was a problem hiding this comment.
The whole objective was rather to rearrange code so that it can be compiled as a module, not to rewrite it. I know about this but it can be rewritten later if necessary.
| ;; Preloaded handlers, subs, functions | ||
|
|
||
| (re-frame/reg-sub | ||
| :get-network-id |
There was a problem hiding this comment.
why this sub is here not in subs?
There was a problem hiding this comment.
because this sub is needed outside of module right after logging in, when the rest of module's namespaces can still wait to be loaded. It could be put into a separate ns, but wouldn't make too much difference.
flexsurfer
approved these changes
Jun 12, 2019
66% of end-end tests have passedFailed tests (16)Click to expand
Passed tests (31)Click to expand
|
cammellos
approved these changes
Jun 12, 2019
|
@rasom would you mind to rebase please? We have new sign txn screen in develop, this is the reason for autotests to fail. Thank you! |
|
@asemiankevich it was rebased before your comment |
68% of end-end tests have passedFailed tests (15)Click to expand
Passed tests (32)Click to expand
|
98% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (46)Click to expand |
100% of end-end tests have passedPassed tests (1) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
please review only the last commit
status-im.screens.ui.network.*was moved tostatus-im.network.ui.*accountsmodule was a bit rearranged in order to fix cyclic depss.utils.handlers -> s.accounts.db -> s.newtork.module -> s.network.events -> s.utils.handlersstatus: ready