Fork me on GitHub
#malli
<
2022-11-26
>
ikitommi10:11:25

what bad can happen if we add a mutable schema type? 🙈 https://github.com/metosin/malli/issues/786#issuecomment-1328020448

pithyless11:11:53

> Just because we can, doesn't mean we should. 🙈 I feel if this was an issue posted on Clojure JIRA, it would require a lot more burden of proof on the poster to argue why this is really necessary, what are the practical use-cases that can't be solved otherwise, and whether the pros overcome all the cons (and future maintenance/compatibility). For example, is breaking the validate/validator referential transparency something we want to be doing so easily? Feels like a slippery slope... I don't have a dog in this fight, I don't really feel the pain of the original poster, but I'd argue prudence: maybe we should move this idea to an independent malli extension library until it shows its usefulness. Otherwise it will be hard to remove if it goes into malli-core and people depend on it. :)

ikitommi13:11:35

I agree it should not be part of the core. I had a use-case in a project with custom reference types, where this might have been useful, not 100% sure. For the “hard to remove later”, there are some escape hatches https://github.com/metosin/malli#alpha: > • extender API: public vars, name starts with -, e.g. malli.core/-collection-schema. Not needed with basic use cases, might evolve during the alpha, follow https://github.com/metosin/malli/blob/master/CHANGELOG.md for details > • experimental: stuff in malli.experimental ns, code might change be moved under a separate support library, but you can always copy the old implementation to your project, so ok to use. maybe malli.experimental.mutable :thinking_face: (NOTE: just added the experimental documentation to README)

ikitommi13:11:40

oh, as the normal schemas capture forms eagerly, the whole parent-schema tree should be mutable to keep the child mutable. Not going to change all schemas to be lazy because of this.

ikitommi13:11:47

mutable = evil. thanks.

ikitommi13:11:57

(deftest schema-test
  (let [!schema (atom :int)
        mutable (mem/schema (fn [options] (m/schema @!schema options)))
        mutable-schema (mem/schema (fn [options] (m/schema [:map [:mutable mutable]] options)))
        immutable-schema (m/schema [:map [:mutable mutable]])]

    (testing "initial schema form"
      (is (= [:map [:mutable :int]]
             (m/form mutable-schema)
             (m/form immutable-schema))))

    (testing "mutating schema"
      (reset! !schema [:enum "so" "mutable"]))

    (testing "mutable schema changed ok"
      (is (= [:map [:mutable [:enum "so" "mutable"]]]
             (m/form mutable-schema))))

    (testing "immutable schema captured the orginal form"
      (is (= [:map [:mutable :int]]
             (m/form immutable-schema)))

      (testing "but workers see the mutated schema"
        (is (true? (m/validate immutable-schema {:mutable "so"}))))

      (testing "mutating schema again"
        (reset! !schema :boolean))

      (testing "form is still from the original value"
        (is (= [:map [:mutable :int]] (m/form immutable-schema))))

      (testing "validator is cached from the second value"
        (is (true? (m/validate immutable-schema {:mutable "so"}))))

      (testing "explainer is cached with the third value"
        (is (nil? (m/explain immutable-schema {:mutable true}))))

      (testing "conclusion"
        (is (not= "mutable" "good idea"))))))

jeroenvandijk12:11:52

@U055NJ5CC Thanks for trying to come up with a solution for #786. I’m reading your conclusion now 😅 Too bad

jeroenvandijk12:11:20

I’ll create something internally and see how this goes. I do have use cases for this / multispec, but I can understand it might not be compatible with Malli’s design

jeroenvandijk13:11:31

Inspired by your example @U055NJ5CC, the following can work for me too

(defn multimethod-schema [mm]
  (let [dispatch (.dispatchFn mm)]
    (m/into-schema
     :multi
     {:dispatch dispatch}
     (map (fn [[type f]] [type (f {dispatch type})]) (methods mm)))))

(defmulti mm-example :Type)   
     
(def mm-schema 
  (if (= (System/getenv "ENV") "PRODUCTION")
    (constantly (multimethod-schema mm-example))
    (fn [] (multimethod-schema mm-example))))

 (defn valid? [x]
   (m/validate (mm-schema) x))

(valid? {:Type :x}) ;=> false

(defmethod mm-example :x [_]
  [:map [:Type [:= :x]]])

(valid? {:Type :x}) ;=> true

🙌 2
jeroenvandijk13:11:18

I don’t think I need much more than this, let’s see

ikitommi13:11:41

that works, unless you wrap it in another schema, .e.g [:map [:mm (mm-schema)]]

ikitommi13:11:04

there, :map eagerly creates the form from the current snapshot value.

ikitommi13:11:38

I think it would be possible (and a good idea!) to be able to disable all caching in malli via an option, e.g. a dev-mode. would allow one to shoot him/her in the leg in new, imaginary ways. But, would also allow a “expected” repl experience, that could be turned of in normal/prod mode.

ikitommi13:11:55

I think it would also clean up malli internals a lot if all caching would happen via the Cached protocol. swapping it’s impl would be trivial after that. so: 1. refactor malli-interanl caching to always use Cached 2. allow disabling caching via an option 3. welcome (optional) mutability for people who don’t have enough troubles already - or they know what they are doing 😎

👍 2
ikitommi13:11:30

I can give it a shot at some point, interested in seeing if 1 would be a good idea.

jeroenvandijk13:11:27

Yeah interesting. To be honest I haven’t used all the features/optimizations of Malli yet. So I can’t judge how far such a change would impact things. I have been viewing Malli naively as an optimized, data oriented version of clojure.spec, but this of course has it’s own tradeoffs. I’m happy to burn myself with the above trick and report later how bad it was 😅

👍 1
jeroenvandijk18:11:16

Btw, I find the :multi construct very powerful. It’s also a nice tool to make the error messages very precise. I already adapted the multispec sugar to something custom to accomodate more precise errors. So maybe a generic multispec thing is not that useful after all. But I believe the option to disable caching for parts of the schema remains interesting. For now though performance is not my main concern