edn-query-language / eql

EQL is a declarative way to make hierarchical (and possibly nested) selections of information about data requirements. This repository contains the base specs and definitions for EQL parsing, AST, etc.
http://edn-query-language.org
MIT License
381 stars 18 forks source link

Do not require spec in core namespace #20

Closed dvingo closed 1 year ago

dvingo commented 1 year ago

For ClojureScript projects that use EQL but do not use spec the EQL core namespace will pull in cljs/spec/alpha.cljs adding 6.23 KB to the resulting payload, this is after using :closure-defines edn-query-language.core.INCLUDE_SPECS false.

This PR moves the specs to a separate namespace where they can be required by those needing them.

awkay commented 1 year ago

The problem with this is that it is a breaking change. Those were public symbols and existing code using this library will break if we make the suggested changes.

It is probably true that there are things in spec itself that cannot be DCE'd.

We can let @wilkerlucio chime in, but the proper (clojure ecosystem) thing to do for this kind of change would be to make new namespaces and leave the old one alone.

dvingo commented 1 year ago

Yea, I realize it is a breaking change, but it's a pseudo-breaking change because you don't have to upgrade versions, and the functionality is not changed, just would require including the new namespace. You also get the compiler to help out with errors like Caused by: java.lang.Exception: Unable to resolve spec: :edn-query-language.ast/node so you would know before runtime if you do attempt to upgrade eql and don't realize this change is needed.

For anyone consuming a library that itself consumes eql, they shouldn't be aware of this change because the library would just include the specs ns for its downstream consumers.

I think the new namespace could be a good compromise too.

wilkerlucio commented 1 year ago

hi @dvingo , like Tony said, at this point, I'm more inclined not to break anything. EQL has been stable for a while, we might consider a different release, but if those 6.23kb are something impactful for your case, you can fork and delete those, and use git deps or something.

dvingo commented 1 year ago

Thanks for the reply @wilkerlucio That approach makes sense to me :)