Fork me on GitHub
#fulcro
<
2020-03-08
>
Jakub Holý (HolyJak)10:03:29

@tony.kay I want to add a check that {} was not forgotten in a query, i.e. that it isn't :query [:some/key (get-query MyChild)] . However I am unsure where to do it: 1. I cannot do it in defsc* because it is inside a macro and has only the raw input form, not the actual data (and may contain function calls such as or ). Perhaps it would be possible to write a spec for it but likely not worth the effort. Or we could write a check using code similar to children-by-prop ... 2. The other option is to check it at runtime instead of macro-expansion time, perhaps reusing the ::eql/query spec for general checks + adding this particular check to it. So (a) do you think that is reasonable? (b) Where to do the check? I guess we only want to run it once.... Thoughts? Thank you!!! (PS: Later, I also want to look into adding a check for using wrongly a (get-initial-state MyChild) inside a template init state. It is somewhat similar problem.)

tony.kay18:03:16

@holyjak thanks for your interest in improving the error messages. I think getting to the 90% would be good enough on the query check, and having it as a compile-time error would be most useful: I think most ppl use the simplified query form (not a fn), so it is my opinion that the check should be in the macro: A keyword followed by a list whose first entry’s name is get-query is an error.

👍 4
Jakub Holý (HolyJak)18:03:04

I know I will make the mistakes again so I'm just protecting myself 😅

Jakub Holý (HolyJak)18:03:04

Wouldn't an additional full runtime spec check (if goog.DEBUG) be useful, though?

tony.kay18:03:09

I’m torn…the answer is “yes, but…”

tony.kay18:03:45

1. spec errors can be really hard to read 2. where do you do it? Probably just at the root…the query could be quite large and the resulting error could be quite opaque and with floating roots you’d have to make sure to hook it up at each root. If you don’t do it at roots, then you have a quadratic performance problem..every call to get-query re-calls the check on every child which in turn runs the check on itself and every child, etc. Not to mention how ugly that mess of error messages gets

tony.kay18:03:11

so, catching common mis-uses at compile time is, I think, more practically useful: 1. They are local to a component 2. The message can be concise and hand-written

tony.kay18:03:17

Fortunately, EQL is pretty easy, right? There just are not very many cases, and I already error-check some of the most common (e.g. mismatching the query/destructuring/initial state)…In early versions that had no checks those were super common mistakes that even I made every day.

tony.kay18:03:49

but the remaining ones are things like you’re naming: a get-query call that is just floating at the top-level of a query, a join with more than one entry (which should never happen inside of a component’s query (but will happen, for unions, AS the query)

tony.kay18:03:24

that’s really it…you have props, joins, and unions…and then possible errors on destructuring and initial state (the latter two are mostly checked).

Jakub Holý (HolyJak)19:03:01

I will think about it and get back to you if I get any good ideas. In the meantime, the defsc check: https://github.com/fulcrologic/fulcro/pull/383

Jakub Holý (HolyJak)20:03:11

@tony.kay Regarding specs, I would solve the problem of re-checking nested queries by not using the eql specs directly but writing "shallow" variants of these; i.e. we care that a join value is a vector (and has (-> % meta :component) ) but otherwise we ignore its content. That way we check only what is relevant for the component at hand. Yes, spec errors are hard to read, but arguably still better than nothing (for catching corner case mistakes that passed through the main compile-time checks). The remaining question is when to check. Ideally we want to check it only once per component class. Perhaps a memoized (defn spec-check-query [component class, query]..)) ?

tony.kay22:03:22

I’m just not interested in a spec check. sorry. Not a good use-case for it. Queries are trivial.

tony.kay22:03:42

I don’t want that much complexity added for something with so little return

👍 4
Jakub Holý (HolyJak)19:03:09

@tony.kay How do you run the clj tests in fulcro from CLI? I see the cljs tests use kaocha but the test alias does not include any runner and I found nothing in the README?

tony.kay19:03:43

see Makefile

tony.kay19:03:55

but just make runs everything

tony.kay19:03:25

clojure -A:dev:test:clj-tests -J-Dguardrails.config=guardrails-test.edn -J-Dguardrails.enabled

👍 4