Fork me on GitHub
#fulcro
<
2019-12-14
>
fjolne04:12:17

@tony.kay Great, thanks for the answers! About mark-missing: I understand the idea, yes, the question is why are nils considered as missing props and how safe it is for me to override this behavior.

fjolne04:12:46

Kinda scared to break some internals with this change, and it might be hard to catch related bugs.

fjolne05:12:03

Well, finally I’ve come to think it’s a bad idea to override a core function of 3 y.o., pretty much certainly would shoot myself in the foot sooner or later.

👍 4
adambros05:12:50

@fjolne.yngling what's the logic behind initializing things as nil?

fjolne05:12:00

I need to distinguish between 3 states of the field: 1) no field => no validation (even if it is declared in form-fields) 2) nil valued field => validation should fail (i.e. marks pre-merge-initialized field as requiring input from the user; consider dropdown or number input, they both don’t have a default empty value, as text input has “”) 3) any other value => validation check

fjolne05:12:25

Well, validation may not fail on step 2, it should depend on your specs. This is more of a question of default empty value for fields which doesn’t have it naturally. Might go with some special keyword, like :my.form/no-value

adambros05:12:16

i feel like you might be complecting the idea behind nil (of no value) and that of being required but currently empty

fjolne06:12:08

Well, for me it seems like I’m trying to decomplect those. Currently, Fulcro’s form-state spec validator checks fields which are really missing as having nil values. But specs describe the field properties, whether the field is required should be a parent spec concern (like s/keys). When loading a form (with subforms) from the server, I might get both an empty form and a thoroughly filled-in one. Whenever I get an empty one, I’m adding fields (and subforms) in pre-merge, but I need to initialize those fields with something. Text input fields are easy, fields with natural / product default values are also easy, but there’re fields like floating point number input, which doesn’t have a default empty value, and generally in Clojure those are considered nils (“no value” === “default empty value” in this case). Thanks for diving in btw;)

adambros06:12:02

i see, it's when you run into form elements that have non-nil default values

fjolne06:12:42

Rather that they don’t have a default value, yes. And I’m trying to choose a good value to mark “no value yet” fields across the whole app. Basically, either I go with nil for this, or with some custom keyword, which is processed as nil by UI inputs/dropdowns/etc

adambros05:12:18

(as context: I wrote this back when fulcro was untangled)

tony.kay06:12:01

@fjolne.yngling it’s not central to any logic in Fulcro…Fulcro is simply trying to make a clear semantic for data merge. If your application logic considers nil a “real” value that the server might return, then you can certainly rework it to be that way. It will not break any other logic in Fulcro, because there is no other logic.

fjolne06:12:20

@tony.kay thanks, good to know

tony.kay06:12:38

we could easily add mark-missing to the app algorithms for pluggable stuff, and make data fetch and mutations allow an override that way

tony.kay06:12:03

the only other place it is used is in merge-component …that one would have to be a named parameter I think, since app isn’t passed in as an argument

tony.kay06:12:32

however, do consider that you already have tri-state field support in form state, so dealing with nil should not be part of validation IMO

tony.kay06:12:46

fields that are incomplete are not validated

tony.kay06:12:03

and once complete, should have a valid value….if nil is allowed then the spec should say that

tony.kay06:12:03

specs do not have a time-based nature to them, which makes them non-ideal for on-the-fly validation

tony.kay06:12:30

they can say what the data should look like when it is “compete”, but intermediate states where you’re trying to validate individual fields just don’t apply

tony.kay06:12:15

so I am having trouble seeing why you’d want a key with a nil value as opposed to a missing key.

tony.kay06:12:51

you’re saying (`s/keys :req [:x])` and then worrying that the spec fails because the required key is missing?

tony.kay06:12:44

in the case of a form (the way the form validation is written) the individual fields can cover the keys case…this isn’t a spec of data flow through function calls…each field has tri-state, and each field can be checked…what is keys helping?

fjolne06:12:39

Currently fs/mark-complete! marks all declared fields as complete, even if they’re actually not in the props. So, they are getting validated against specs as having nil values. I generally don’t have fields which could potentially be nil (due to the nature of Datomic).

tony.kay06:12:06

but why are you marking them all complete in that case?

tony.kay06:12:27

why are you not just marking the ones that you have?

fjolne06:12:15

I’m just using mark-complete on the root form, it’s convenient for large forms with a lot of subforms, to mark complete on “save” button press.

tony.kay06:12:20

it’s a trivial helper to write…it’s just the :complete? set in the form config

tony.kay06:12:32

please don’t mix “convenient” with operational correctness. mark-complete lets you say exactly what you consider complete (one at a time, or all at once)…you choosing not to say is a convenience, but is not a limitation. Using that in reduce or doseq is trivial

tony.kay06:12:35

but if you do want to consider nil a legal value, then I’m glad to take a PR for a pluggable mark-complete.

tony.kay06:12:53

it’s a very simple thing to add in 🙂

fjolne06:12:45

Well, “convenient” was a wrong wording, sorry. I need to mark all present fields as complete, but mark-complete marks all declared fields.

tony.kay06:12:05

then write the helper…it’s like 4 lines of code

tony.kay06:12:15

mark-present-fields

tony.kay06:12:45

there’s even an fs/update-forms helper that will walk nested forms for you

fjolne06:12:10

Sure, I’m fine with writing the helper (actually I’ve already done it before the discussion), just wonder whether I’m misunderstanding something.

tony.kay06:12:22

It’s your data model 🙂

tony.kay06:12:00

you’re right that mark-missing does expose an opinion about nil punning that isn’t necessarily consistent…but then since when is nil punning consistent in Clojure? 😜

tony.kay06:12:36

(str nil) (name nil) (namespace nil) (map + nil)

tony.kay06:12:40

half of those crash

tony.kay06:12:22

it is a personal pet peeve of mine, so I feel your pain, but I fear there is no universal fix

tony.kay06:12:44

(though I do think nil should never cause crashes)

tony.kay06:12:25

if I had the time, I would have already forked clojure and fixed error messages and crashes on nil punning 😜

tony.kay06:12:15

and then there’s the “select your poison” for nil as in: keep vs. map…this is just more of that. sometimes you just need an option

fjolne06:12:47

Yeah, there’s pretty much of it in the core, but we’re kind of all in the same boat and everybody is used to those things, whereas when you go to some 3rd-party library you don’t really know what to expect, so kinda like learning the new language. Thanks for your time, it’s been insightful!

fjolne18:12:50

@tony.kay jfyi it turned out that approach with not marking absent fields as complete makes the validation of the root form into a constant :unchecked state, cuz merge-validity has these rules:

* :unchecked :valid = :unchecked
  * :unchecked :invalid = :unchecked
so I backed off to re-implementing no-spec-or-valid? to check for (contains? props key); otherwise I’d basically re-implement half of the form state logic

fjolne18:12:32

also the new-old pre-merge logic (as of 3.0.15) stopped eliding nil values (no sweep anymore), so the question about overriding mark-missing isn’t relevant now