Clojurians Log v2

@dnolen

clojure -Sdeps '{:deps {org.clojure/clojurescript {:git/url "<https://github.com/clojure/clojurescript>" :sha "42bcb07b8bf23d57f98e4617e4c4c93347f09715"} reagent/reagent {:mvn/version "1.0.0-alpha2"}}}' -m cljs.main --install-deps

because in the code path that the patch touches npm-deps will be nil rather than a map

in this case I also get an error on earlier versions of clojurescript, but at least it's shelling out to npm

npm ERR! 404 Not Found - GET <https://registry.npmjs.org/@cljs-oss%2fmodule-deps> - Not found

I finally got around to trying out emitting require statements from a macro, the thing we talked about a while back (https://clojurians-log.clojureverse.org/cljs-dev/2020-05-29/1590760413.380700)

ran into two issues, the first one is small and I can submit a patch for that https://github.com/plexus/require-from-macro-test

the second issue is that the required namespaces are not actually getting compiled. Is that something that could/should work, or is it up to me to do that as part of the macro?

require needs to be completely static and cannot be emitted dynamically like that. no macroexpansion takes place when the files are first read to gather the ns data.

require was only meant to work in files that don't have a ns in the first place but are still completely static

@thheller David specifically suggested trying this approach, see the referenced conversation

yeah I saw. to get this working however you need to change a bunch of machinery and I'm not sure calling the compiler API from within a macro isn't going to lead to crazy issues.

https://clojure.atlassian.net/browse/CLJS-3273

I guess we're about to find out 🙂 this issue is for the first issue which I think is straightforward, and is separate from the question how/where/if those required namespaces should get compiled.

@plexus ok, make a issue w/ that repro - I guess I haven't run into this because I don't run that with :npm-deps empty

@thheller I wasn't suggesting dynamic require, that can never work

@plexus the other issue is interesting, I would say unexpected since original use case (`user.cljs` and other free floating scripts) should trigger compilation of required namespaces? Any issue should talk about this more fundamental problem

well at what point is it a "dynamic require"? I'd say the example from Arne is dynamic

that is, it should in fact work (`ns*` adding things to compilation work)

@thheller dynamic how? it's happening from a macro, not at rutnime

as in you need to macroexpand to discover new requires "dynamically". who knows what other side-effects those macroexpansions may trigger

not a big deal

we already have stuff like that - as long as people understand the limitations doesn't seem like there's anything to be concerned about

i.e. mostly useful for testing - which is what @plexus was wanting this for

I understand but it makes things really annoying for tools (eg. Cursive) but also the build aspects like caching, parallel-compile, etc

for testing use can't matter since this is at the entry point

well not actually sure about Cursive. maybe it already expands macros.

nor parallel compile

all existing limitations apply, it's gotta be a require right after ns form

so I don't see it adding much complexity either

I think for entry points it's actually probably something of a win

since we're never going to do custom reader conditionals

so this approach could get you other benefits to configure a build at the entrypoint

since you can't do this stuff in ns directly

interestingly ...

given all the restrictions it might be possible to solve the caching issue?

that would be a big win, since then you're not limited to entry point

I don't think that solving build related things in side-effecting macros is a good idea

to be clear - this isn't new stuff anyway

how people use stuff that should work is up to them - we can make it better

heck even a simple CLJ function that just returns a "dynamic" ns form that tools can invoke to figure out the "entries" is probably better

like I said - not interested in new ideas

just making sure there's a way with stuff we've already done

most of what @plexus has mentioned are just bugs

so unless I'm completely out of touch with the CLJS codebase there is still a parse-ns that reads forms only until it encounters none ns or ns*. it then stops and analzyes the deps it found

and then starts actual compilation

parse-ns is just a general helper

analyze uses, compile uses, the build stuff uses it

but yeah, that's how it works

ok, I'll make a ticket for the :npm-deps issue, I'm not sure what the conclusion is for compiling the required files. Do I get it right that you're saying it is intended to work, and the current behavior is a bug?

well I guess the only issue I have with this is that is breaks some assumptions about caching I have built into shadow-cljs (eg. if the file doesn't change its ns info and requires don't either)

but I guess I can easily adapt that or just automatically opt of of caching in files that have ns* forms

@plexus well what I want to know is why this doesn't already work

i.e. you make a file a w/o a namespace that (require ...) at the top

but it maybe opens the doof for some more "dynamic" require and not solving those via build config (eg. (require-in-dev-only '[re-frame-10x.whatever :as x]))

that should already work

@thheller yes, I'm not saying you can't carried away but I think there are some easy problem you could solve w/o build tool support

@plexus the whole point of (require ...) was so that you could compile scripts that don't declare ns

so I'm assuming we tested this at some point so maybe there's a narrower thing that's not working here

right but in this case there's ns + require

ok so that's what I want to know 🙂

ns to pull in the macro which then does the require

minimal case showing require works fine, and ns + require doesn't

the macro stuff doesn't matter really

assuming that this is still the relevant code then it just stops when it finds ns and doesn't look further https://github.com/clojure/clojurescript/blob/f5f9b79f6f446cef768e190971cf67fc78bf5b93/src/main/clojure/cljs/analyzer.cljc#L4439

or if the macro part does matter - of course include that

sure

@thheller oh right

https://github.com/plexus/require-from-macro-test/blob/main/src/foo/main.cljs

I see

the issue is that ns stops, ns* doesn't

or perhaps ns* star does too - anyways don't remember

@plexus let me know 😉

ns* recurs so that continues

so that's the issue, I think if you solve that, that ns will continue if ns* exists

it will work

well then it always analyzes the second form in each file and immediately discards it in most cases which may cost a bit of perf. probably not a big deal though.

yeah, I think reading a few more form probably isn't going to matter much though

cool, thanks! I'll try to come up with a patch

👍:skin-tone-4:

@thheller definitely looks like your CLJS patch yesterday was fine and our CI setup is borked - I think what happened was that if the cache expires then WebKit master gets pulled down? will try to repro that locally and then start pinning WebKit to a known good sha that we can bump periodically

yeah I saw. failure looked too weird to be related to my patch 😛

I've added a separate ticket for the CI issue and was also going to take a peek if nobody else gets to it first: https://clojure.atlassian.net/browse/CLJS-3274

@mfikes thanks!!!

FWIW, CI is still passing in the older Travis-based tests which also make use of JSC, so a JSC regression on master (or whatever is being used in GitHub actions tests) seems to be a good theory.

@mfikes were you pinning to a version? or to a build you had made and reusing?

@dnolen Maybe this pins it sufficiently: https://github.com/clojure/clojurescript/blob/master/.travis.yml#L14

ah huh, you know I couldn't get that to work which is why I used the OS X runner

@mfikes fwiw, it might be nicer to track WebKit since then we can test against newer JS features when that makes sense?

my impression is that version of WebKit in gtk might be old?

given that this literally worked last week

maybe we should just need to specify that sha to that time

Yeah... I'm also running a side test in CI to see what JSC is actually returning from tap>. Maybe we can catch something ahead of time and file a ticket with JSC

adding a recur to cljs.analyzer/parse-ns for :ns makes the basic case of (ns foo.bar) (require 'foo.baz) work, I'll submit a patch for that. The next problem is that when creating the require via a macro the macroexpansion fails, get-expander* fails to find the macro var

if I change it to use a requiring-resolve then it works, so this is the last thing blocking me. suggestions on how to actually fix this are welcome.

@dnolen here's the ticket for --install-deps https://clojure.atlassian.net/browse/CLJS-3275

might be enough to change it to (or (nil? npm-deps) (map? npm-deps))

@plexus patch for that welcome

@plexus further back - finding what macro var?

a snippet would clarify things a bit

(ns foo.bar (:require-macros [foo.baz :refer [macro-that-requires]]))
(macro-that-requires)

that should solve itself if foo.baz does the proper self require and you just (ns foo.bar (:require [foo.baz :refer [macro-that-requires]]))

oh wait no. it doesn't actually analyze the deps at that point

@plexus but what is not being resolved? the macro?

yes, the macro is not loaded yet when it's trying to be expanded, this causes an NPE

(.findInternedVar ^clojure.lang.Namespace
              #?(:clj (find-ns nsym) :cljs (find-macros-ns nsym)) sym)

@thheller the self-requires works in that the compilation succeeds, but the build is invalid, the namespace that the macro requires isn't compiled

where exactly are macro namespaces loaded? does the analyzer do that?

(signing off soon but I'll continue with this tomorrow. thanks for all the help!)

@plexus ah right, there's the ns-side-effects pass in the analyzer

FWIW, the CI tests are failing in JSC because it returns undefined for setTimeout. Details in https://clojure.atlassian.net/browse/CLJS-3274

interesting, i'm seeing SO posts saying that JSC doesn't implement setTimeout, setInterval, and clearTimeout