Fork me on GitHub
#cljs-dev
<
2018-09-29
>
herald17:09:14

I'm looking into this https://dev.clojure.org/jira/browse/CLJS-2917 and it looks like the error is caused because cljs.closure/compile-file receives a File as output-file, instead of a relative path output-file is #object[java.io.File 0xfabe3f8 /tmp/out3012973275628785939777962205402581/cljs/user/user358016D.js] instead of a string like clojure/browser/repl/preload.js. Where I'm stuck is, should compile-file be able to handle an output-file that is a File (and then use that instead of creating a new File) or has something gone wrong further back and it should have been passed the string cljs/user/user358016D.js instead?

mfikes17:09:47

@regen Yeah, that's a good question. I wouldn't go so far as to conclude that it need to be a relative string, as a relative File would also work. To be honest, I think you could realistically debate whether the fix should take the form of making cljs.repl/load-file pass a relative path, or making compile-file be more forgiving, accepting absolute paths (perhaps checking that they are in the output dir as a sanity check).

mfikes17:09:56

Not exactly the same problem, but I had solved a similar issue downstream in this ticket https://github.com/mfikes/ambly/issues/127

herald20:09:59

Thanks for your thoughts, @mfikes. I went a bit back and forth, wrote a fix that passed it as a relative path string, but then I found out that cljs.repl.rhino, nashorn, node and graaljs all do the same thing: using cljs.closure/src-file->target-file for output-file to compile-file. (I guess that means that these other repl's would fail the same way if used with a temp out dir?) With this and the issue you posted in mind, it seems to me that making compile-file more forgiving is the solution.

👍 4
thheller20:09:28

should it be considered a bug or a feature that the CLJS ns parser seems to accept this?

thheller20:09:21

all usages I ever saw followed :include-macros with true? is my understanding just wrong?

mfikes20:09:57

Yeah, true should go there

mfikes20:09:30

I’m AFK, but it appears that the ns spec would catch this, as it has

(s/def ::include-macros #{true})

thheller20:09:02

seems like it doesn't since people use that library and it has been that way since Jan 2017

mfikes20:09:09

In hindsight, that spec could be simplified to use true?

mfikes20:09:58

Yeah, I’d previously discovered you could put any truthy value there, but perhaps it doesn’t even care if a value is there

thheller20:09:24

I use a custom ns parser based on spec in shadow-cljs which doesn't accept this

thheller20:09:45

not sure if I should adjust the parser since that would make it an "official" feature

thheller20:09:50

but it looks like a bug to me

mfikes20:09:20

(doc ns) specifies :include-macros takes true

mfikes20:09:46

My take is that the posh code is invalid. The ns form parser could always catch more corner cases, but perhaps enabling cljs.core.specs.alpha is a better long-term solution (especially given we will also soon have a compiler option to disable macro spec checking — which may also apply to the hack we are doing for the ns special form)