arichiardi / replumb

ClojureScript plumbing for your self-hosted REPLs.
http://clojurescript.io
Eclipse Public License 1.0
210 stars 16 forks source link

module resolution sometimes doesn't work #128

Closed jaredly closed 8 years ago

jaredly commented 8 years ago

For example, quil.core depends on org.processingjs.Processing, which replumb thinks should be at /org/processingjs/Processing.clj{c,s}, but it's actually at /processingjs.js. Now, google closure knows this, and if you look up goog.dependencies_.nameToPath['org.processingjs.Processing'], it has the right path. In fact, it has all the paths you need for dependency resolution -- are there any reasons not to use it?

arichiardi commented 8 years ago

Maybe it is a bug, I am checking

arichiardi commented 8 years ago

Yes it is a bug, and exactly here.

jaredly commented 8 years ago

So the fix I did was replace this line with:

(conj
  (load/file-paths-for-load-fn src-paths macros path)
  (aget (.-dependencies_.nameToPath js/goog) (str name)))

... but now I'm getting other (seemingly unrelated?) errors about parsing ns declarations in quil's macro files... so maybe it worked, maybe not? At least that file (processingjs) is being loaded correctly now

arichiardi commented 8 years ago

The goog dependencies graph is generated at init time and actually the problem is that we need to read from it even when the regexp does not match goog. A discriminant is that we need to load js deps when we are loading an import but we don't have this info in *load-fn*

arichiardi commented 8 years ago

We can directly query the graph for the name we are passed into *load-fn* in the case instead of using the regexp

arichiardi commented 8 years ago

So this becomes:

;; we don't need the if anymore
(get-goog-path name) (load/read-files-and-callback! verbose?
                                                                       (load/file-paths-for-closure src-paths goog-path)
                                                                       read-file-fn!
                                                                       cb)

If you make this change and open a PR I will add tests ok?

jaredly commented 8 years ago

+1

arichiardi commented 8 years ago

So this is a bit more complicated than I thought cause processingjs is an external lib...I think we don't have at the moment a way to process external libraries in replumb, maybe exposing :foreign-libs would trigger something but we still need to manually load them in our bootstrapped mode before passing them down the line.

So the solution is not even close. We should instead probably have a new case that triggers if the extension is .js that reads the file from load-fn -> :path returns it as :source.

This should work.

arichiardi commented 8 years ago

So it is even more complicated because there is no mapping 1:1 from js files to namespaces in Google Closure, so we will probably need to parse deps.js (we do it already for goog js files) and create an indexed map for external libs, probably exposing :foreign-libs like for the compiler...digging...

jaredly commented 8 years ago

wait but there is a mapping, it's goog.dependencies_.nameToPath <- that's "namespace -> js file".

arichiardi commented 8 years ago

Does it work reliably with any js required library? I guess so...I will investigate more, for sure it would solve a lot of problems

tomasz-biernacki commented 8 years ago

(aget (.-dependencies_.nameToPath js/goog) (str name)) actually yields the correct path for both goog files and non-goog files... but in the whole cond we also need to take into account the evaluation cache (which is on #127)

jaredly commented 8 years ago

and actually, I can sidestep this by doing

(swap! cljs.js/*loaded* conj
       'org.processingjs.Processing
       )

and there's no benefit from parsing the js file anyway, so it's not too much of an issue

arichiardi commented 8 years ago

Yes that's true @jaredly but technicahlly we are missing a case :) Maybe @mfikes has some wise suggestion here ;)

mfikes commented 8 years ago

@arichiardi mfikes/planck#121 is essentially the same kind of issue, I think

arichiardi commented 8 years ago

The problem is the same, and @mfikes' solution works for cljsjs deps (because they contain a deps.js file in the associated jar). If we don't have it, well @tomasz-biernacki is investigating as we speak :smile:

tomasz-biernacki commented 8 years ago

@jaredly First of all, going back to your first example: /org/processingjs/Processing.clj{c,s} are not the only paths because also the ".js" file is searched (first the "cljs", then "cljc" and last "js").

Second, this is a file (as far as I understand) produced during compile time, meaning that you have a dependency in you project, you require it somewhere, you compile the project, the file gets copied to out and then you try to require it from the REPL, is this correct? So a hack (similar to what you tried to do) is to use .nameToPath, something along the lines of this, something that maps the symbol org.processingjs.Processing to the correct file path. And that's because it is so in the package.

Third, if you want to require a file that was not generated during compilation we have two options, either:

I hope this is clear :D

arichiardi commented 8 years ago

@jaredly Can you try (when you have time) if 0.1.5-SNAPSHOT solves the issue? @tomasz-biernacki introduced :foreign-libs for cases like yours, test and example here.

If you are ok with it you can also close this issue :wink:

arichiardi commented 8 years ago

Closing this.