search

RyanMcG / lein-npm

Manage Node dependencies for CLJS projects
295 stars 43 forks source link

resolve-in-jar-dep is a wee bit brittle #14

Open RyanMcG opened 10 years ago

RyanMcG commented 10 years ago

I received the following error message when running lein deps.

java.lang.ClassCastException: clojure.lang.PersistentVector cannot be cast to clojure.lang.Named
 at clojure.core$name.invoke (core.clj:1518)
    leiningen.npm.deps$resolve_in_jar_dep.invoke (deps.clj:51)
    clojure.lang.AFn.applyToHelper (AFn.java:160)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invoke (core.clj:628)
    clojure.core$partial$fn__4230.doInvoke (core.clj:2470)
    clojure.lang.RestFn.invoke (RestFn.java:408)
    clojure.core$keep$fn__6402.invoke (core.clj:6713)
    clojure.lang.LazySeq.sval (LazySeq.java:40)
    clojure.lang.LazySeq.seq (LazySeq.java:56)
    clojure.lang.RT.seq (RT.java:484)
    clojure.core$seq.invoke (core.clj:133)
    clojure.core.protocols$seq_reduce.invoke (protocols.clj:26)
    clojure.core.protocols/fn (protocols.clj:53)
    clojure.core.protocols$fn__6031$G__6026__6044.invoke (protocols.clj:13)
    clojure.core$reduce.invoke (core.clj:6287)
    leiningen.npm.deps$resolve_in_jar_deps.invoke (deps.clj:63)
    leiningen.npm.deps$resolve_node_deps.invoke (deps.clj:82)
(let [jar-project-entry (.getEntry jar-file "project.clj")
      jar-project-src (when jar-project-entry
                        (read-string                                        ; 1
                          (slurp                                            ;
                            (.getInputStream jar-file jar-project-entry)))) ;
      jar-project-map (when jar-project-src
                        (->> jar-project-src (drop 3) (apply hash-map)))
      jar-project-name (when jar-project-src
                         (name (second jar-project-src))) ; 2
      ...

  1. This may not actually get a form with defproject being invoked in it. I've done this before:

    (read-string (str \( a-string-of-clojure-code \)))

    Of course, this will change result in a list of forms so some searching needs to take place to find the defproject usage.

  2. Here we assume, we've found a totally normal defproject, we call second and name to get a string representing the name of the project. Unfortunately, for a project.clj like reply's this fails and breaks the whole thing.

    (let [dev-deps '[[speclj "2.7.2"]
            [classlojure "0.6.6"]]]

(defproject reply "0.3.5-SNAPSHOT" ...

I am not convinced that pulling node dependencies of maven dependencies is the right thing to do. I'd like to get a better understanding of why this is done. Regardless, I think and ideal solution would put the dependencies in the pom and interpret them from there instead of trying to read project.clj.

A good, short term solution would be to provide better error handling so that when a project.clj is improperly interpreted, it is simply ignored. Another, would be to surface an option which disables gathering node-dependencies on sub projects. I think there are cases where it is undesired.

I would be happy to submit a pull-request once we know what might work best!

RyanMcG commented 10 years ago

I'll post a PR with a fix for the assumptions made about the forms in project.clj soon, it's already done, it's just on my personal computer.

RyanMcG commented 10 years ago

ping

radhikalism commented 10 years ago

As a sidenote, leiningen 2.4 begins to deal with a similar problem, which supposes a general solution for project map processing that might apply to lein-npm.

The newly introduced 'release' task involves reflecting on the contents of project.clj, and Phil H notes that a general 'change' task for reading and rewriting project maps might eventually be useful. http://librelist.com/browser//leiningen/2014/5/1/release-task/

If that ever materializes as a robust API in leiningen, it could be reused by plugins that extract or inject values like lein-npm does. So I don't know if transmitting data via the pom file is the best course of action.

+1 for this PR.

RyanMcG commented 10 years ago

@arbscht I may be mis-interpretting you or the linked post to the leiningen mailinglist. But this seems to be referring to leiningen tasks which rewrite project.clj. Though I think this could be useful for leningen or lein-npm I am not sure it is solving the problem I am bringing up here.

Our issue is with reliable reading a project definition of a dependency. This is because (defproject ...) is code. Through the beauty of LISP we are able to read this trivially, but not necessarily perfectly (we've the PR I've already posted is a good example of how making assumptions about the structure of project.clj can bite us). I do not think we have to wait for some yet-to-be-implemented change task. Leiningen already provides functions for reading project.clj files. I feel like we should probably use one, if we can't read pom files directly. (Maybe this will be my next PR)

I was originally under the impression that just reading pom files would be a significant performance improvement, but I don't think that is the case anymore.

radhikalism commented 10 years ago

@RyanMcG You're right, of course. I just mean that working with project.clj and not pom files is likely to be a better long-term direction, as this is where all the action happens in a leiningen context (including unrelated rewrites, eventually). The pom file should probably remain an implementation detail for leiningen itself to manage, even for lein plugins, rather than making it a leaky abstraction.

Using leiningen's read for project files sounds like an excellent idea — if it is now in a workable state. I forget now why it wasn't sufficient when I last looked at it, but there have been many changes to that namespace since then.

RyanMcG commented 10 years ago

@arbscht Alright, I agree. We should avoid relying on pom and go the read route, if it works.

@bodil thoughts on #15?

bodil commented 10 years ago

I think this project needs a maintainer (I'm not doing Clojure anymore, so not able to support it). Anyone interested?

RyanMcG commented 10 years ago

I am. Full disclosure, I might ask for some non-clojure-specific library maintenance help though.

I'd be happy to co-maintain with someone else too. We could have a cljs-npm organization or something.

bodil commented 10 years ago

OK, how about I just transfer it to you, then you're free to create an organisation if more people want to pitch in?

radhikalism commented 10 years ago

Excellent, thanks @RyanMcG!

(I'm very much interested in this project but cannot help as a maintainer at this time. Happy to contribute and support otherwise.)

RyanMcG commented 10 years ago

@bodil transfer away!

bodil commented 10 years ago

Transfer in progress!

bodil commented 10 years ago

Oh, actually, @RyanMcG, you'll need to delete your fork first, getting "ryanmcg/lein-npm already exists" sort of errors.

RyanMcG commented 10 years ago

@bodil done!

Well, I renamed my current fork at least, but that should fix it.

bodil commented 10 years ago

Apparently not.

"RyanMcG already has a repository in the bodil/lein-npm network"

RyanMcG commented 10 years ago

OK, deleted. Take 3. Sorry about that. https://github.com/RyanMcG/lein-npm 404s now instead of redirecting

bodil commented 10 years ago

Seems to have worked. Enjoy the repo. :)

RyanMcG commented 10 years ago

Ha, thanks.

RyanMcG commented 10 years ago

Well, I am going to merge in #15 then.

olivergeorge commented 9 years ago

I think this bug renders lein-npm incompatible with chestnut.

RyanMcG commented 9 years ago

@olivergeorge can you elaborate? Have you tried current master?

olivergeorge commented 9 years ago

Hi Ryan

Just tried "0.5.0-SNAPSHOT" and that works.

Test was:

cheers, Oliver

RyanMcG commented 9 years ago

OK, so it sounds like I should cut a release. I'll try to do that in the next couple of days. On Dec 26, 2014 4:04 PM, "Oliver George" notifications@github.com wrote:

Hi Ryan

Just tried "0.5.0-SNAPSHOT" and that works.

Test was:

  • lein new chestnut npmtest

  • add npm plugin and node-dependenies to project.clj based on readme

  • lein npm install
  • See error with 0.4.0

  • update npm plugin reference to 0.5.0-SNAPSHOT

  • lein npm install
  • See install work

cheers, Oliver

— Reply to this email directly or view it on GitHub https://github.com/RyanMcG/lein-npm/issues/14#issuecomment-68158054.

olivergeorge commented 9 years ago

Brilliant, thanks.

On 27 December 2014 at 08:39, Ryan McGowan notifications@github.com wrote:

OK, so it sounds like I should cut a release. I'll try to do that in the next couple of days. On Dec 26, 2014 4:04 PM, "Oliver George" notifications@github.com wrote:

Hi Ryan

Just tried "0.5.0-SNAPSHOT" and that works.

Test was:

  • lein new chestnut npmtest

  • add npm plugin and node-dependenies to project.clj based on readme

  • lein npm install
  • See error with 0.4.0

  • update npm plugin reference to 0.5.0-SNAPSHOT

  • lein npm install
  • See install work

cheers, Oliver

— Reply to this email directly or view it on GitHub https://github.com/RyanMcG/lein-npm/issues/14#issuecomment-68158054.

— Reply to this email directly or view it on GitHub https://github.com/RyanMcG/lein-npm/issues/14#issuecomment-68159297.

RyanMcG commented 9 years ago

@bodil I'm trying to cut a release but I am getting an "Access denied" error.

Can you add me to the lein-npm group on clojars? https://clojars.org/groups/lein-npm

Thanks :smile:

bodil commented 9 years ago

Oops, sorry, was sure I already had.

RyanMcG commented 9 years ago

@bodil that did it! @olivergeorge release cut!

olivergeorge commented 9 years ago

Thanks Ryan.

Seems like a interesting time for CLJS with JS dependencies.

What are you making of the JS build conversations happening at the moment? David's raised a ticket about tweaking :foreign-libs and boot-cljsjs was announced recently. Sounds like Google Closure is working towards AMD support too. Related discussions happening on the mailing list.

On 29 December 2014 at 07:58, Ryan McGowan notifications@github.com wrote:

@bodil https://github.com/bodil that did it! @olivergeorge https://github.com/olivergeorge release cut!

— Reply to this email directly or view it on GitHub https://github.com/RyanMcG/lein-npm/issues/14#issuecomment-68219247.

RyanMcG commented 9 years ago

No problem.

Honestly, I haven't looked into much. Any chance you can link me that ticket?