Clojurians Log v2
fulcro
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
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.)
@tony.kay here is a first draft. What do you think? https://github.com/fulcrologic/fulcro/pull/464
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 .... đ
> 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).
> 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.
Thank you! Continuing the discussion there.
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
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?
I would avoid adding any kind of checks there, because its a performance critical path
I think checking the spec is better
it can get you detailed information about the error
EQL provides full specs for the whole syntax, that's the one I'm talking about
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) ast (try (expr->ast k)
(catch ...
(throw ex-info "Invalid key in a join" {:key k, :join x})))we can measure and try something like that, but I still think if the purpose is validation, using the spec is a better path
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.
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.
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
I agree that âď¸ would be really nice. Perhaps @tony.kay 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...
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....?
I was thinking a manual call to (s/explain ::eql/query query), something on that direction in places where the user introduce queries
Thank you. What do you think about improving the error messages? Still not persuaded?