<< Back to previous view

[DZIP-5] Make it compatible with ClojureScript Created: 01/Nov/15  Updated: 24/Nov/15

Status: Open
Project: data.zip
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Artem Yarulin Assignee: Aaron Bedra
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File data.zip-cljs-added-2.diff     File data.zip-cljs-added-3.diff     File data.zip-cljs-added.diff    


It would be really cool to have this library for ClojureScript as well. Currently ReactNative is rising and ClojureScript which has full support of it, will benefit from this lib.

I'm new to the Clojure world, but for me it looks like we can simply rename src/*/.clj files to *.cljc and support both environments.

What do you think? Is this a right approach?

Comment by Artem Yarulin [ 01/Nov/15 1:32 AM ]

Forgot to mention: First http://dev.clojure.org/jira/browse/DZIP-4 has to be resolved

Comment by Artem Yarulin [ 01/Nov/15 1:04 AM ]

OK, this is not that easy:
For example https://github.com/clojure/data.zip/blob/master/src/main/clojure/clojure/data/zip/xml.clj#L41-43:

^String (apply str (xml-> loc zf/descendants zip/node string?))
(str "\\s" (char 160) "+") " "))

Has to be replaced with clojure.string/replace and who knows what else.

Can you point me a good example of Clojure/ClojureScript library? Obviously we have to run all the tests for CLJS version as well, but in my projects I'm using projects like doo for that, not sure that this is a right approach.

Comment by Alex Miller [ 01/Nov/15 7:11 AM ]

Changing the files to cljc means that this library would then require Clojure 1.7. Generally we try not to be eager about having such a recent release be the minimum requirement.

There are several contrib projects that have both clojure and clojurescript support although the build process is kind of a pain for all of them. core.logic is one example.

Comment by Artem Yarulin [ 04/Nov/15 8:16 AM ]

Can we make an exception from this rule? Last commit to this library was on May 21, 2014, so in most cases this library is done and doesn't require any new functionality.

With *.cljc files it will be so much easier to create universal libraries.

Comment by Alex Miller [ 04/Nov/15 8:25 AM ]

I'm not ready to make 1.7 a minimum requirement for this library yet. While doing a split build with duplicated code for clj and cljs would be more complicated, I think that is preferable for the time being.

Comment by Artem Yarulin [ 04/Nov/15 8:36 AM ]


1 create a folder main/clojurescript/clojurescript/data
2 copy files from Clojure and change ext to cljs
3 change namespace from clojure.data.zip.xml to clojurescript.data.zip.xml to refer folder change
4 make needed changes (remove any interop to Java world, like .replaceAll)
5 change pom.xml to lein compatible project.clj and build CLJS version using cljsbuild (like in core.logic)
6 make the same for tests (folder creation + copy + rename + remove interop)
7 run tests like in core.logic (https://github.com/clojure/core.logic/blob/master/script/test)

Is this a right approach?

Comment by Alex Miller [ 04/Nov/15 9:09 AM ]

Yes, that's basically it. Based on the direction of other libraries, I think it would be better to leave the namespaces the same (for future cljc-ification).

Comment by Artem Yarulin [ 04/Nov/15 9:10 AM ]

Ack, thx. I'll try to do this during this weekend

Comment by Artem Yarulin [ 09/Nov/15 3:12 AM ]

Here the patch which adds CLJS support.

While I followed the workflow that we discussed before I have couple of questions:

  • Could you please verify project.clj and pom.xml. I don't know what pom.xml is used for, but I've tried to merge all the information from it to project.clj. Also I increased a version to 0.1.3
  • zip.cljs and xml.cljs is pretty much the same as clojure version. Only one change is replacing .replaceAll with clojure.string/replace function call
  • xml_test.cljs - as data.xml not yet ported to CLJS I've replaced test atom with already created map representation of xml doc
  • I found a script/test in core.logic and decided to include it here as well as I think it could be used for CI. Currently the easiest way to test it is: lein test && lein cljsbuild once adv && node resources/tests.js
Comment by Alex Miller [ 09/Nov/15 8:29 AM ]

I won't have time to look at the patch right now, but answering your questions:

  • pom.xml is the Maven build - this is what actually produces the release artifact (the project.clj is a convenience but the pom.xml is actually the more important project file). Don't modify the version - the release process does this automatically.
  • is it possible to also have the .clj version use clojure.string/replace instead of .replaceAll?
  • we don't currently run lein at all during the release build, but it is useful for verification, so happy to have the script/test.
Comment by Artem Yarulin [ 09/Nov/15 8:57 AM ]

No problem - whenever you have time.

1) I'll revert version
2) Sure thing - I'll make the same change
3) kk

I'll create a new patch then

Comment by Artem Yarulin [ 10/Nov/15 2:04 PM ]

Fixed all the issues you mentioned:
1) Version reverted to the original one
2) string/replace used in clojure version as well

BTW - lein re-generated pom.xml file and I included changed file as well. Not sure if this is a right thing

Comment by Alex Miller [ 10/Nov/15 2:06 PM ]

Please don't re-generate the pom file - it is the primary build definition (the project.clj is secondary).

Comment by Artem Yarulin [ 11/Nov/15 9:49 AM ]

Changes to pom.xml removed

Now it should be pretty much done

Comment by Artem Yarulin [ 22/Nov/15 12:08 AM ]

Sorry for the annoyance, don't know how fast things are happening here.

Is there anything I can do to push this patch forward? Create example project, more docs, etc.?


Comment by Alex Miller [ 24/Nov/15 10:09 AM ]

Nope, just waiting on me to have time.

Generated at Tue Nov 24 16:14:34 CST 2015 using JIRA 4.4#649-r158309.