Clojurians
#cljs-dev
<
2017-07-16
>

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

dnolen00:07:18

definitely possible - tests welcome :slightly_smiling_face:

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’s 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’s no :file)

anmonteiro00:07:10

right that never worked before

anmonteiro00:07:14

but how would it work now? :slightly_smiling_face:

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 :slightly_smiling_face:

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’re 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’s 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’s fixed

anmonteiro00:07:07

@dnolen it’s 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’t know about foreign libs

anmonteiro00:07:11

every other test we have works because it didn’t exercise requiring foreign-libs

anmonteiro00:07:38

if you comment the changes I did to cljs.closure you’ll see the tests failing with an error that it can’t 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’re doing something here which doesn’t 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’s where we build the index

anmonteiro00:07:37

you’re totally right

anmonteiro00:07:48

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

anmonteiro00:07:54

but we’re 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’ll close the ticket

dnolen00:07:56

well that test is still a good test :slightly_smiling_face:

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’t 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 :disappointed:

dnolen00:07:57

heh, really it’s more all the good work our testing is doing :slightly_smiling_face:

dnolen00:07:06

I’m going to punt on this overrides enhancements for now, it’s a little more subtle to do correctly than I’m 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’s 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’t 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’s assembling their own compiler env

anmonteiro06:07:32

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