Upgrading schema library
Closed this issue · 11 comments
Hello! i apologize for not mentioning to this to Clara directly in advance, but I've made a framework called Precept that wraps Clara in eav tuple syntax and is presently aimed to support client-side development. I'm trying my best to give Clara its due credit, and I am very grateful for the hard work you've put in to make it possible to run a fully-fledged rule engine in the browser. I'd appreciate any feedback you could provide if and whenever you have the time.
Ok, so I'm wondering if you want to consider upgrading your version of schema to 1.1.6. It seems Clara's version breaks for users of Clojure 1.9.0-alpha17 and Clojurescript 1.9.562. Our tests pass with the old schema version but we get errors running a CLJS app.
I'm not necessarily opposed to making this change. The schema version is starting to lag and there have been quite a few changes. https://github.com/plumatic/schema/blob/master/CHANGELOG.md#111 is the most aggressive one I've seen so far. It actually broke something I was working on that had a clash of versions of schema both on the classpath (AOT-drama).
To get a big more background info though, I find a few points of interest here:
-
Is schema "1.0.1" reported to break in current versions of cljs and clj? I don't see any reference to this. It is odd that it is breaking on a
schema.core/validatecall on a basic persistent map impl. -
The cljs warnings have all been mentioned and addressed in recent schema versions according to the changelog's https://github.com/plumatic/schema/blob/master/CHANGELOG.md
-
Am I correct that https://github.com/CoNarrative/precept is not actually using the schema dependency directly (it only is pulling it transitively)? I do not see it require'd in any of the namespaces there.
I'm double checking on that point, just to determine if the fix seems inappropriate. If you don't use the schema lib at all, you shouldn't have to be concerned with it.
@mrrodriguez Correct. The PR would be adding it in order to support the newer versions of CLJ/CLJS. We don't use it for anything.
On using Clara in Precept, no problem. Glad it is useful. :)
On the Schema version, the proposal is just to update the version Clara uses in its own project.clj, not change Clara in a way that would break usage of the current version (e.g. by using new features in Schema), correct? Clara built on my machine when I bumped the Schema version to 1.1.6 without any additional changes and from the Schema changelog I don't see anything between 1.0.1 and 1.1.6 that looks like it would break Clara's usage of Schema.
I think an upgrade sounds reasonable. My only other point though is I don't really understand why the precept error was happening. I don't see anything in schema that was broken and fixed between these versions.
I would expect the same error in a project that uses that version of Schema and the latest versions of Clojure and Clojurescript. I thought maybe Clojure.spec was the culprit but I've no clear reason for thinking that. The errors I coped in here reference Schema.spec and the accumulators namespace.
I think the relevant change in Schema is PR 393; basically a symbol Schema defines is now a symbol in core.cljs after clojure/clojurescript@78891af for CLJS-2013. The failure @alex-dixon pointed to contained
No protocol method Schema.spec defined for type cljs.core/PersistentArrayMap
so it seems likely that this is related to the MapEntry issue although I haven't dug into the Precept code that failed.
@WilliamParker I mentioned in my original comment above #306 (comment) on point (2) that several warnings have been addressed.
The exception doesn't appear to be the same as those warnings though. That is where I was surprised because I had not heard of anything actually broken in any recent versions schema against any recent versions of cljs. The no protocol impl on PersistentArrayMap is strange. It has been impl'ed on that data type for quite a while, including the version Clara is running
https://github.com/plumatic/schema/blob/schema-1.0.1/src/cljx/schema/core.cljx#L763-L764
So I was curious if there was some sort of cross-dependency interaction or AOT-related thing going on here that is actually the source of trouble. It seems increasing the version helps, but that could be due to these integration sorts of interactions.
I'm not greatly concerned with Clara having the version changed or anything like that. Getting rid of the warnings is nice and also there were some changes to help with some hard to diagnose AOT issues from before (I've seen them before too and understand it, but it isn't fun).
So there are good things going on there.
@mrrodriguez looking at that again I don't think I'd expect failures either. I also see that there was some discussion about another user having warnings not an error in the Precept PR. Are there steps that can reproduce this issue?
Regardless of this, getting rid of ClojureScript compiler warnings is a good thing so I'm inclined to merge #311 unless there are objections. I'll plan to look over the changes in Schema more closely before doing so though to make sure I'm not missing any breaking changes.
+1 to the schema uplift regardless.
It'd be nice to reproduce for better understanding, but that doesn't have to prevent it.
There doesn't seem to be any objection of bumping our Schema version so I have merged #311
Thanks!