Fork me on GitHub
#fulcro
<
2021-03-05
>
souenzzo13:03:25

From #cljs-dev The new release of #clojurescript will use a newer closure-library that introduced some breaking changes.

WARNING: Use of undeclared Var goog.debug.Logger.Level/getPredefinedLevel at line 36 /home/travis/build/chkup/fulcro/src/main/fulcro/logging.cljc
Wrong number of args (1) passed to goog.log/getLogger at line 46 /home/travis/build/chkup/fulcro/src/main/fulcro/logging.cljc
This was detected by the canary, more info/full logs here: https://github.com/cljs-oss/canary/tree/results/reports/2021/03/05/job-001708-1.10.835-715cdc07#-fulcro

❤️ 3
👍 3
tony.kay00:03:54

This is on Fulcro 2

tony.kay00:03:35

I’m not actively maintaining F2. If you want to submit a patch to fix this, please branch from the fulcro-2 branch, and send a PR targeted at that branch.

tony.kay00:03:22

You’ll need to upgrade dependencies and test it. If you can certify that it looks good, I’ll push an update to clojars.

Jakub Holý (HolyJak)21:03:29

What is the best way to capture errors caused by invalid EQL? Currently If a component has the :query [:some/prop {}] , this is logged in the Console: > error when calling lifecycle function com.example.client/refresh > #error {:message "Invalid expression ", :data {:type :error/invalid-expression}} which is not very helpful. Is it possible to turn on Spec checking of queries? Is it a good idea? (This error is thrown during app/set-root! , perhaps somewhere in merge-alternate-unions.)

wilkerlucio21:03:05

you could, but I would try to be precise about when to checking for it, the check can get expensive so you don't want to be re-doing it

Jakub Holý (HolyJak)21:03:11

Ok, thank you!!! Perhaps a better solution would be to fix EQL to provide a better error message. I looked at it but unsure about how best to do it. This throws not on {} but on the first entry's key, which is nil . Perhaps it would help to wrap the failing https://github.com/edn-query-language/eql/blob/master/src/edn_query_language/core.cljc#L177ast in the parent join->ast> with try-catch and rethrow a more informative message such as "Invalid empty join {}". But still not perfect b/c it does not tell us where that invalid join was. Good luck searching for {} in the code 🙂 Any thoughts?

wilkerlucio21:03:57

I would avoid adding any kind of checks there, because its a performance critical path

wilkerlucio21:03:05

I think checking the spec is better

wilkerlucio21:03:11

it can get you detailed information about the error

wilkerlucio21:03:48

EQL provides full specs for the whole syntax, that's the one I'm talking about

Jakub Holý (HolyJak)22:03:04

I do not mean adding any checks, I mean adding one or few try-catch-rethrow with more context. Those only add overhead when the error occurs, I believe. So from

 ast         (expr->ast k)
to
 ast  (try (expr->ast k)
       (catch ...
          (throw ex-info "Invalid key in a join" {:key k, :join x})))

wilkerlucio22:03:26

we can measure and try something like that, but I still think if the purpose is validation, using the spec is a better path

Jakub Holý (HolyJak)22:03:44

My purpose is to provide more useful error message to the user. Currently the whole Fulcro app blows up with little info (other than the unhelpful message and that it failed during init and that EQL was involved). Providing a better err message with more context would be valuable.

Jakub Holý (HolyJak)22:03:58

I am making https://github.com/holyjak/fulcro-troubleshooting to help catch various issues but this happens in a way that is impossible for me to check for. (I would need to ask the user to manually run a function prior to calling app/set-root! , which is too cumbersome.) If there was a place where I could automatically hook into the lifecycle before the app blows up then validation would be an option but as it seems now, it is not.

wilkerlucio22:03:05

I agree a better error reporting is wanted, I think we just having different opinions on how to do it, since EQL is such a base library, I will always prefer a path that doesn't change it, in this case the spec is such path, this could be a change in Fulcro to validate the query at some strategic places, this way we can get detailed spec information for reporting back. a drawback of my approach is requiring pulling spec, but I believe it can be done just at dev time, which is the more critical spot

Jakub Holý (HolyJak)22:03:20

I agree that ☝️ would be really nice. Perhaps @U0CKQ19AQ has some input here? But I think it would nevertheless be valuable to improve error reporting in EQL since all users - not just Fulcro - would benefit from it and I do not see any performance impact and quite negligible complication of small parts of the code. Of course, YMMV. I would be willing to help...

Jakub Holý (HolyJak)22:03:57

If I want to involve Spec - do you have an example how to? Do I instrument the whole EQL namespace or just some key fns....?

wilkerlucio22:03:55

I was thinking a manual call to (s/explain ::eql/query query), something on that direction in places where the user introduce queries

👍 3
Jakub Holý (HolyJak)23:03:10

Thank you. What do you think about improving the error messages? Still not persuaded?

tony.kay00:03:10

I think it is reasonable to add some kind of dev-time query check to Fulcro. I think the two critical paths that would not affect performance much would be at initial mount (which would get it on hot code reload too) and in set-query!. The latter one is used mainly by the dyn routers, so it would only cause a penalty on route changes.

tony.kay00:03:51

I’m also feeling like we should have better errors around missing :initial-state. That seems to trip people up a lot. It would also be possible to add macro checks to defsc that could find stupid errors on compile. It can’t expand the get-query calls, but it could look for obvious mistakes, like a call to get-query that isn’t embedded as a value in a map, a map that has more than one key, and other such obvious mistakes.

tony.kay00:03:52

the check, for that matter, could sub calls to functions that have get-query in their name with [:subquery-of-ComponentClassName] and run a spec check on the resulting expression.

tony.kay00:03:47

of course such a check would have to “bail” if there were other function calls in there

tony.kay00:03:21

and syntax quoting from user space would cause possible problems….but I think you could catch a lot of common cases

tony.kay00:03:25

@U0522TWDA you have any interest in working on that? I’d be glad to give you some pointers if you’re willing to do the legwork/testing

Jakub Holý (HolyJak)00:03:52

Speaking about the dev-time query check on mount etc. would it be (s/explain ::eql/query query) ? As explained above, I think we would also need to check during app/set-root! b/c it is called before mount and fails it there are errors. BTW how do we know it is dev time? goog.DEBUG?

tony.kay01:03:08

I think the error message would be better if we just did a valid? and indicated that the query was not valid EQL. We might use explain-data from there and try to narrow it down, but I would not use explain. (when goog.DEBUG …) is the proper wrapping for it, since that will cause dead code removal in advanced compile. And yeah, set-root! seems a good place as well. In the macro, I was thinking of replacing any function calls whose symbol name is get-query as I explained above, then look for any other things that are lists that start with symbols. Then, if there are NO other symbols (recursive check) in the query, then it should be ok to check the resulting EQL and throw a syntax error. That would be way more noticeable than an error in the console.

tony.kay01:03:47

I’m considering adding a default initial state with a join on every query edge for components with constant idents…if you wanted to try your hand at that as well 😄

tony.kay01:03:27

hm, not sure that one is possible. It’s hard to detect if the ident is constant…nvm

tony.kay01:03:15

some kind of runtime check around initial state seems like a good idea, I’m just not sure what it is. I can’t tell you how many times someone’s error/question is purely a matter of they didn’t set up initial state for their component.

Jakub Holý (HolyJak)07:03:47

>. I can't tell you how many times someone's error/question is purely a matter of they didn't set up initial state for their component. When is that a problem? I thought it only is critical for https://book.fulcrologic.com/#_a_warning_about_ident_and_link_queries and router targets (and all their ancestors)?

Jakub Holý (HolyJak)16:03:31

I'd also appreciate your feedback on the root query runtime check here https://github.com/fulcrologic/fulcro/pull/465 I am struggling to get the check run upon refresh (i.e. RAD demo's com.example.client/refresh ) Obviously I do not understand which code path is taken .... 🙏

tony.kay04:03:25

> When is that a problem? I thought it only is critical for Beginners will fail to compose initial state, but correctly compose the query and UI. As a result their component will not receive props. If they are using a component that has a constant ident and they’ve interacted with the component then they will see that component’s ident-based table entry in a table, but it will not be joined to the graph (since their mutation that they ran is what put the table entry there). So, it looks like the component’s state is in the db, but props are nil. Simply looking at the data path through the db shows the problem: the join from the parent had nothing in state (because there was no initial state).

tony.kay04:03:59

> I am struggling to get the check run upon `refresh` Easier to discuss this kind of in the issue, I think. I’ll comment there.

Jakub Holý (HolyJak)08:03:02

Thank you! Continuing the discussion there.