Fork me on GitHub
#cljs-dev
<
2017-07-16
>
dnolen00:07:18

definitely possible - tests welcome 馃檪

anmonteiro00:07:10

fixed, working on a test

dnolen00:07:34

working on an enhancement after encountering an annoyance with testing :global-exports

dnolen00:07:00

previously :foreign-lib overrides had to be keyed on :file but that鈥檚 a strange requirement

dnolen00:07:11

instead we should key on :provides

anmonteiro00:07:16

^ what does this change?

anmonteiro00:07:23

looks the same to me?

dnolen00:07:31

@anmonteiro it never worked before

dnolen00:07:56

(notice there鈥檚 no :file)

anmonteiro00:07:10

right that never worked before

anmonteiro00:07:14

but how would it work now? 馃檪

anmonteiro00:07:36

rely on an upstream declaration and allow users to provide their global exports?

anmonteiro00:07:43

^ foreign lib regression fix

dnolen00:07:12

@anmonteiro it did work before 馃檪

dnolen00:07:32

local :foreign-libs could override upstreams

anmonteiro00:07:19

but they had to provide a file, right?

anmonteiro00:07:24

and you鈥檙e making it possible not to

anmonteiro00:07:30

by keying on provides

dnolen00:07:55

yes because file is a horrible key

dnolen00:07:10

you could have different artifacts that provide the same ns

dnolen00:07:27

@anmonteiro there鈥檚 no context on CLJS-2249

dnolen00:07:34

neither in the ticket, the patch, or the test

dnolen00:07:40

I have no idea what鈥檚 fixed

anmonteiro00:07:07

@dnolen it鈥檚 the same fix you did to make analyze-deps happy for non-node targets

anmonteiro00:07:30

we need to add js-dependency-index to the compiler env

anmonteiro00:07:49

or it won鈥檛 know about foreign libs

anmonteiro00:07:11

every other test we have works because it didn鈥檛 exercise requiring foreign-libs

anmonteiro00:07:38

if you comment the changes I did to cljs.closure you鈥檒l see the tests failing with an error that it can鈥檛 find the foreign lib that the test requires

dnolen00:07:45

@anmonteiro hrm yeah I think this patch is strange?

dnolen00:07:00

because you鈥檙e doing something here which doesn鈥檛 have anything to do w/ js modules

dnolen00:07:24

in my :foreign-libs work I realized that cljs.build.api/build is broken

dnolen00:07:33

that鈥檚 where we build the index

anmonteiro00:07:37

you鈥檙e totally right

anmonteiro00:07:48

we should add js-dependency-index to the compiler-env no matter what

anmonteiro00:07:54

but we鈥檙e not

anmonteiro00:07:05

so your fix probably supersedes this one

dnolen00:07:07

so let me just provide a fix for that

anmonteiro00:07:14

yeah, I鈥檒l close the ticket

dnolen00:07:56

well that test is still a good test 馃檪

dnolen00:07:58

and I want to make sure this small tweak makes that test pass

dnolen00:07:06

@anmonteiro try your test against master

dnolen00:07:55

reopened with comment

dnolen00:07:13

the issue was correct, I think the re-shuffling for modules broke this

dnolen00:07:49

sadly hacking on ClojureScript and needing to run tests ain鈥檛 exactly good for battery life

anmonteiro00:07:16

yeah the test passes now

anmonteiro00:07:19

attaching a revised patch

anmonteiro00:07:27

re: battery life, I bet the JVM drains it pretty quickly 馃槥

dnolen00:07:57

heh, really it鈥檚 more all the good work our testing is doing 馃檪

dnolen00:07:06

I鈥檓 going to punt on this overrides enhancements for now, it鈥檚 a little more subtle to do correctly than I鈥檓 interested in considering - but will come back to it later

mfikes00:07:46

Fork the repo and push your changes to your fork and let Travis take the hit on amp-hours.

dnolen00:07:12

that鈥檚 a really good idea for the return trip

dnolen01:07:06

:global-exports seem to work perfectly fine at the REPL

dnolen01:07:32

before my battery dies, cool to note that this is the most active period in ClojureScript development since the beginning, REPL overhaul, & bootstrapping https://github.com/clojure/clojurescript/graphs/contributors

anmonteiro06:07:56

@dnolen follow up fix to the JS dependency index thing https://dev.clojure.org/jira/browse/CLJS-2251 I don鈥檛 think we should force every caller of cljs.env/default-compiler-env to pass in the full options (with implicit options added)

anmonteiro06:07:36

my patch addresses that by bringing back the call to updating the compiler env with the js-dependency-index at that point which fixes every upstream call of cljs.closure/build, whatever the arity

anmonteiro06:07:00

pretty sure the way you handled it in current master is a breaking change to everybody that鈥檚 assembling their own compiler env

anmonteiro06:07:32

(added a new test case too that demonstrates the issue - another arity and 馃挜 )