Fork me on GitHub
#cljs-dev
<
2017-01-31
>
juhoteperi00:01:27

Resolving relative filepaths against classpath directories is easy when using clojure.java.classpath

juhoteperi00:01:46

But what I think is even simpler, is just using foreign-library :file for output path if it is set and relative

anmonteiro01:01:36

@juhoteperi I'm not sure that's a good solution though

juhoteperi01:01:29

I tried to think a case where it wouldn't work but couldn't come up with one

anmonteiro01:01:41

from the little testing I did so far, using a relative path doesn't seem to work when consuming node modules

anmonteiro01:01:22

which is why the guide uses getAbsolutePath in this section: https://clojurescript.org/guides/javascript-modules#node-modules

anmonteiro01:01:41

I haven't looked at the code to know why that's the case though

anmonteiro01:01:16

your patch wouldn't break anything, I suppose, but the above use case wouldn't be possible with Boot?

juhoteperi01:01:18

Hmm strange, but, that would still work with my change, as it checks if the :file is relative before using that

juhoteperi01:01:53

IF the node_modules is outside of source/resource-paths, I think absolute path would work with Boot

anmonteiro01:01:16

ah that's right

juhoteperi01:01:26

But if node_modules are inside source/resource-paths, or a task adds them to the fileset, absolute paths won't work

juhoteperi01:01:59

-source calls -url and -url always coerces paths and files to URLs, which will make them absolute

juhoteperi01:01:37

So I think classpath relative paths should work in that example

anmonteiro01:01:00

hrm, so there's this problem in several Node modules in that they rely on JS hoisting of functions and use a variable before it has been defined in the fiel

anmonteiro01:01:38

the patch I submitted to React addresses that for React's particular case, but the Closure Compiler doesn't rely on hoisting of those exported functions

anmonteiro01:01:56

this causes undefined reference errors

anmonteiro01:01:19

I'm not sure if this should be addressed in GCC or by every module author out there

anmonteiro01:01:24

this issue seems related but addresses a different consequence: https://github.com/google/closure-compiler/issues/2240#issuecomment-273946670

anmonteiro04:01:38

@juhoteperi btw, there's a new option :closure-module-roots that Boot may have to set if using relative paths? I'm just speculating because I don't know what it does

juhoteperi08:01:58

@anmonteiro Doesn't affect the logic to build relative output paths for foreign modules at all. I think it is just for Closure compiler.

dnolen13:01:08

@anmonteiro that’s just to get Google Closure to resolve modules correctly

thheller14:01:50

@dnolen trying to wrap my head around some spec stuff in cljs, I don't quite understand why this swap! is there

thheller14:01:17

as def-impl overrides it eventually

thheller14:01:28

is that a self-host related thing?

dnolen14:01:14

@thheller def-impl doesn’t override it - that’s runtime

dnolen14:01:28

Clojure spec doesn’t have this compile-time/run-time problem

thheller14:01:03

yes I know but it is the only time cljs.spec/runtime-ref is accessed (from the cljs/spec.cljc that is)

thheller14:01:20

cljs/spec.cljs has its own

thheller14:01:34

which is confusing me 🙂

dnolen14:01:01

it might be used at some point in the future when we want the compiler/analyzer to look at it

thheller14:01:16

ok thought there is some hidden self-host magic I didn't know about

thheller14:01:11

hmm that does not seem compatible with caching?

dnolen14:01:47

it’s not something I intend to think about right now

thheller14:01:51

well nvm .. a problem for the future 😛

niwinz16:01:04

I have started getting warnings like this with the latest (1.9.456) cljs compiler:

WARNING: Use of undeclared Var uxbox.main.ui.users/js at line 21 /home/uxbox/uxbox/frontend/src/uxbox/main/ui/users.cljs
WARNING: Use of undeclared Var uxbox.main.ui.users/js at line 21 /home/uxbox/uxbox/frontend/src/uxbox/main/ui/users.cljs

niwinz16:01:01

the code is a macro that internally emits code with js/React.... stuff

niwinz16:01:49

something with js/ ns has been changed?

anmonteiro16:01:56

Interesting, I noticed that too in Devcards but I didn't isolate the problem and thought I was doing something wrong

niwinz16:01:49

That does not happens with 1.9.293

anmonteiro16:01:23

Right. What I said is meant to support your conclusion too

anmonteiro16:01:46

I just didn't report it because I was messing around with new JS module stuff

dnolen16:01:39

somebody should make something minimal and file an issue

dnolen16:01:45

not going to look at any of these libs 🙂

dnolen16:01:43

likely a trivial fix, based on what people are showing so far

anmonteiro17:01:43

@dnolen I got Om Next working with React from node_modules last night https://github.com/anmonteiro/om/commit/686921531c7bcbbd75804e6b9240b12d92eaaa5f This made me think about one issue with the current node_modules support, which is: how do library authors lock in a particular version of, say React?

dnolen17:01:12

Not in scope really

anmonteiro17:01:14

and 2) how do library users override that dep at their own risk?

dnolen17:01:30

This feature is for apps not libs

anmonteiro17:01:44

right, that also makes sense

anmonteiro17:01:07

so library authors would develop against something that users would need to provide?

anmonteiro17:01:15

just like it works in the JS world

dnolen17:01:24

Might change my mind about that but it needs work hammock

Roman Liutikov17:01:21

@anmonteiro I also built Rum/Sablono app with React from NPM. Couldn’t use a number of other deps directly from NPM because they all have stupid errors like the one you’ve reported in React recently 😞

anmonteiro17:01:41

I had the same problem yesterday at work too

anmonteiro17:01:53

wanted to try and use something from node_modules but it had the order issue

anmonteiro17:01:07

I think this is ultimately an issue with GClosure

anmonteiro17:01:52

@roman01la I've opened an issue there and Chad acknowledged it, so we might see a fix soon (Chad is the person who's been working on all the JS module support PRs for GClosure)

Roman Liutikov17:01:49

@anmonteiro following JS quirks is a bad idea, bad it would really help :thinking_face:

anmonteiro17:01:54

@roman01la fortunately linters can detect that particular case now

anmonteiro17:01:32

(even if they don't fix it in Closure)

dnolen17:01:30

@anmonteiro if we end up having deep support for Node.js deps, it’ll come after we solve the direct CommonJS require problem

dnolen17:01:04

but yes I’ve considered having support for writing package.json deps in deps.cljs or something like that

anmonteiro17:01:10

right, that makes sense

anmonteiro17:01:35

^ but it's really hacky and very brittle

anmonteiro17:01:02

needless to say these are only experiments, I have only been tinkering with it and not using it anywhere

dnolen17:01:29

I’m not fundamentally against assuming the existing of node

dnolen17:01:38

and shelling out to do our work as we’re doing now for the other bits

anmonteiro17:01:18

@dnolen that doesn't seem wrong to me, people can still use the compiler without Node if they don't use this functionality

anmonteiro17:01:32

and if they want to consume Node modules, they'll have it installed anyway 🙂

anmonteiro18:01:01

@jr @niwinz do any of you have a repro of the js warning?

anmonteiro18:01:06

I can't seem to repro it anymore

jr18:01:14

let me try deleting my analysis cache

jr18:01:24

I couldn't repro it in a new repo

dnolen18:01:55

@jr it may be an analysis cache problem, so useful to determine if it only happens w/ cache

jr18:01:21

the warning goes away after deleting my analysis cache

jr18:01:40

probably related to the tag changes

dnolen18:01:20

yeah we write out tag info into the cache for return types, so maybe the problem is there somehow

jr18:01:24

I can consistently compile now without a warning so doesn't seem like an issue

anmonteiro18:01:51

could it be caused by the upgrade?

anmonteiro18:01:02

i.e. new CLJS trying to read an analysis cache written by old CLJS?

anmonteiro18:01:35

that makes sense to me, because I can't repro on this computer after a fresh clone

anmonteiro18:01:48

but it happened to me on my other computer which already had the repo

juhoteperi18:01:35

@anmonteiro Did you try Schema? I got error about RegExp from Schema with Boot-cljs (so no cache)

anmonteiro18:01:55

might be a different issue altogether

anmonteiro18:01:00

do you have a repro?

juhoteperi18:01:16

Just a work project depending on Schema, nothing more specific

jr18:01:28

boot-cljs caches analysis in ~/.boot/cache. I had that same RegExp warning which is now gone

juhoteperi18:01:55

@jr No it doesn't

juhoteperi18:01:42

Huh, the warning is gone for me too

dnolen20:01:24

so bug or no bug?

dnolen20:01:53

is this because people switched ClojureScript compiler version without starting a clean build?

dnolen20:01:07

(shouldn’t be necessary but if it doesn’t work in this case would be good to know)

anmonteiro20:01:03

seems like that's what happened

anmonteiro20:01:17

although you're not really supposed to "clean" your build if you're using boot

dnolen20:01:46

well ok, one less thing to worry about then

juhoteperi21:01:14

Really strange

juhoteperi21:01:21

Well, Boot uses JVM pid to generate different temp dirs for each JVM process.. so it is in theory possible that new processes would have got same pid as previous Boot process

juhoteperi21:01:53

I don't see other ways Boot-cljs could have cache left around from previous runs

juhoteperi21:01:30

Oh, and Boot cleans the temp directory when starting

juhoteperi21:01:02

Anyway, can't reproduce

anmonteiro21:01:52

btw, boot-cljs is under the Boot org now?

juhoteperi21:01:48

@anmonteiro Repo yes, I'm probably keeping the group and artifact-id same as I can't decide on new ones

anmonteiro21:01:06

what's wrong with boot/boot-cljs?

juhoteperi21:01:52

Currently boot org is only for the core libs, though I guess that's not necessarily a problem. Another problem is that the tasks use group-id.artifact-id naming convetion for the main namespace, and boot.boot-cljs is not very good name... Perhaps it should just be boot-cljs.core.

juhoteperi21:01:10

Naming is hard and renaming existing things is extra hard 😞

niwinz23:01:35

@anmonteiro Hmm, on my case it is not cache

niwinz23:01:00

just because I have disabled cache-analysis

anmonteiro23:01:37

@niwinz awesome, do you have a minimal repro?\

anmonteiro23:01:48

we haven't been unable to repro it

niwinz23:01:04

Hmm not now, I'll try to take some time tomorrow for it.

niwinz23:01:05

it is pretty big but just downloading and setting the new cljs version on project and execute ./scripts/dist-main should be enough to see that warnings

niwinz23:01:58

In any case, tomorrow I'll try to create a more smaller repo with and try to reproduce that.