Introduce malli library #17867
Merged
Introduce malli library #17867
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
ilmotta
requested review from
alwx,
cammellos,
rasom,
vkjr,
FFFra,
flexsurfer,
qfrank,
yqrashawn,
tumanov-alex,
Parveshdhull,
briansztamfater,
ajayesivan,
smohamedjavid,
clauxx and
J-Son89
ilmotta
commented
Nov 10, 2023
| malli.core malli | ||
| malli.dev.pretty malli.pretty | ||
| malli.dev.virhe malli.virhe | ||
| malli.error malli.error |
There was a problem hiding this comment.
FYI: I added these aliases because there's a tendency devs would copy from tutorials alias abbreviations like mu to mean malli.util, or me to mean malli.error.
Jenkins BuildsClick to see older builds (14)
|
ilmotta
commented
Nov 10, 2023
|
|
||
| (def view | ||
| (quo.theme/with-theme | ||
| (schema/instrument #'view-internal ?schema))) |
There was a problem hiding this comment.
FYI: This is the functional approach with schema/instrument. Notice the need to var quote view-internal. This has been added to the schema guidelines too.
ilmotta
commented
Nov 10, 2023
|
|
||
| (schema/=> format-amount | ||
| [:=> [:cat [:maybe :int]] | ||
| [:maybe :string]]) |
There was a problem hiding this comment.
FYI: The macro approach with schema/=> is more convenient when the function to instrument is not wrapped in higher-order functions (which is the case for quo components wrapped in quo.theme/with-theme).
|
Thanks for the detailed pr. Great work @ilmotta! 🙌 |
cammellos
reviewed
Nov 10, 2023
|
|
||
| (def view (theme/with-theme view-internal)) | ||
| (def ?schema |
There was a problem hiding this comment.
maybe it could be defined above the usage? it makes it a bit more clearer if reading it the first time, but no strong opinion
There was a problem hiding this comment.
I have no preference @cammellos, and since nobody objected to your suggestion I'll apply it.
Edit: resolved in 292a6ad
cammellos
approved these changes
Nov 10, 2023
I believe it caught something already, the component test wasn't passing all the parameters required :) |
FFFra
approved these changes
Nov 10, 2023
|
This is amazing! Can't wait to try out! Thank you a lot @ilmotta |
clauxx
approved these changes
Nov 10, 2023
ilmotta
force-pushed
the
malli-core-foundations
branch
from
bd5b69b
to
c974afc
Compare
78% of end-end tests have passedFailed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
Hi @pavloburykh. When you tested this PR build with Issue 1 fixed, you did that using the same commit I cherry picked to the PR you mentioned #17919. I just merged that one, and then rebased this malli PR against Therefore, I think we're safe to merge without further manual tests. Time to merge 🚀 |
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
…17919) In PR #17867 we have a namespace named schema.core, but this namespace is taken by library prismatic/schema already (see https://github.com/plumatic/schema/tree/master/src/cljc/schema), a library used by our direct dependency on bidi 2.1.6. This leads to a broken build where the ClojureScript compiler reports undeclared vars (https://clojurescript.org/reference/compiler-options#warnings). We change the order Java resolves dependencies via the classpath mechanism. We now first resolve our own Clojure sources, and then project dependencies.
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
This commit is the foundational step to start using malli (https://github.com/metosin/malli) in this project. Take in consideration we will only be able to realize malli's full power in future iterations. For those without context: the mobile team watched a presentation about malli and went through a light RFC to put everyone on the same page, among other discussions here and there in PRs. To keep things relatively short: 1. Unit, integration and component tests will short-circuit (fail) when inputs/outputs don't conform to their respective function schemas (CI should fail too). 2. Failed schema checks will not block the app from initializing, nor throw an exception that would trigger the LogBox. Exceptions are only thrown in the scope of automated tests. 3. There's zero performance impact in production code because we only instrument. Instrumentation is removed from the compiled code due to the usage of "^boolean js.goog/DEBUG". 4. We shouldn't expect any meaningful slowdown during development. **What are we instrumenting in this PR?** Per our team's agreement, we're only instrumenting the bare minimum to showcase 2 examples. - Instrument a utility function utils.money/format-amount using the macro approach. - Instrument a quo component quo.components.counter.step.view/view using the functional approach. Both approaches are useful, the functional approach is powerful and allow us to instrument anonymous functions, like the ones we pass to subscriptions or event handlers, or the higher-order function quo.theme/with-theme. The macro approach is perfect for functions already defined with defn. **I evaluated the schema or function in the REPL but nothing changes** - If you evaluate the source function, you need to evaluate schema/=> or schema/instrument as well. - Remember to *var quote* when using schema/instrument. - You must call "(status-im2.setup.schema/setup!)" after any var is re-instrumented. It's advisable to add a keybinding in your editor to send this expression automatically to the CLJS REPL, or add the call at the end of the namespace you are working on (similar to how some devs add "(run-tests)" at the end of test namespaces). **Where should schemas be defined?** For the moment, we should focus on instrumenting quo components, so define each function schema in the same namespace as the component's public "view" var. To be specific: - A schema used only to instrument a single function and not used elsewhere, like a quo component schema, wouldn't benefit from being defined in a separate namespace because that would force the developer to constantly open two files instead of one to check function signatures. - A common schema reused across the repo, like ":schema.common/theme" should be registered in the global registry "schema.registry" so that consumers can just refer to it by keyword, as if it was a built-in malli schema. - A common schema describing status-go entities like message, notification, community, etc can be stored either in the respective "src/status_im2/contexts/*" or registered globally, or even somewhere else. This is yet to be defined, but since I chose not to include schemas for them, we can postpone this guideline.
This was referenced Jan 2, 2024
ilmotta
added
malli
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.
Summary
This PR is the foundational step to start using malli in this project. It is not a small PR, and some parts of the code are not trivial. All this is necessary to guarantee our hot-reload, testing and REPL workflows are kept intact and enjoyable. Special thanks to @J-Son89, @clauxx, and @yqrashawn for helping review, test and propose improvements to this PR's branch.
Please, take in consideration we will only be able to realize malli's full power in future iterations. Do expect exciting ideas from other collaborators! Too many to spoil now ;)
For those without context: the mobile team watched a presentation about malli and went through a light RFC to put everyone on the same page, among other discussions here and there in PRs.
To keep things relatively short:
^boolean js.goog/DEBUG.Demo
In this demo we see a live experience without hot-reload or REPL involved, just pressing things around. As the instrumented function (
stepcomponent) is called, when an invalid input argument is passed, a red warning is automatically displayed, telling the developer to look at the logs. When the same function is called again with the correct arguments, the error is removed from the screen.Without a mechanism to warn the developer, and since we don't throw exceptions to avoid the LogBox, the developer would frequently forget to fix schemas, increasing the likelihood of merging with such problems.
The automatic error removal only works if using
schema/instrumentbecause we can then wrap the original function using a HoF. Only in future endeavors we'll be able to achieve this effect with the macro approach, because we cannot just wrap a var and usemalli.core/=>. We're considering submitting a PR to malli as well.demo-malli.webm
What are we instrumenting in this PR?
Per our team's agreement, we're only instrumenting the bare minimum in this PR, to showcase 2 examples.
utils.money/format-amountusing the macro approach.quo.components.counter.step.view/viewusing the functional approach.Both approaches are useful, the functional approach is powerful and allow us to instrument anonymous functions, like the ones we pass to subscriptions or event handlers, or the higher-order function
quo.theme/with-theme. The macro approach is perfect for functions already defined withdefn.I evaluated the schema or function in the REPL but nothing changes
schema/=>orschema/instrumentas well.schema/instrument.(status-im2.setup.schema/setup!)after any var is re-instrumented. It's advisable to add a keybinding in your editor to send this expression automatically to the CLJS REPL, or add the call at the end of the namespace you are working on (similar to how some devs add(run-tests)at the end of test namespaces).Where should schemas be defined?
For the moment, we should focus on instrumenting quo components, so define each function schema in the same namespace as the component's public
viewvar.To be specific:
:schema.common/themeshould be registered in the global registryschema.registryso that consumers can just refer to it by keyword, as if it was a built-in malli schema.src/status_im2/contexts/*or registered globally, or even somewhere else. This is yet to be defined, but since I chose not to include schemas for them, we can postpone this guideline.How do I learn malli?
Malli's README is fairly complete. The REPL can be your best friend in this journey. Ping other devs with more context, pairing can be much more effective :)
Steps to test (devs only)
We suggest you at least checkout the branch and try to modify the
quo.components.counter.step.view/?schemato see how malli works on a basic level. If you are a REPL user, please report if you faced any issue 🙌status: ready