I discovered something interesting related to cljs.js/eval-str. When the the supplied *load-fn* uses core.async’s go macro things get corrupted

This is due to the use of binding inside eval-str and binding and core.async don’t play well together.

It might worth documenting it

@dnolen what do u think?

@viebel there’s no issue

async stuff is async

bindings aren’t going to work

Yeah but when someone uses eval-str he is not aware that the code uses binding

@viebel this is not a problem

This is why I think we need to document it

I’m not going to say anything more about this 🙂

whether we document or not, ok - but this is not a priority

I know that binding and async will (never) be compatible. Would u like a patch with a word about it in documentation of *load-fn*

to be honest I’m not that interested

talking about core.async is of course irrelevant

if anything is going to be documented it should probably be in the ns docstring

simply enumerating the bindings established by the fns in that namespace

putting anything about binding and async in this namespace specifically is not a good idea

it’s a fundamental limitation which has nothing to do with bootstrapped

the right place to cover this is probably binding

Ok.

The thing is that it’s very natural to use go inside custom *load-fn*...

@viebel I’ve said what I’m going to say about this

so my plan today is to fix http://dev.clojure.org/jira/browse/CLJS-1821

http://dev.clojure.org/jira/browse/CLJS-1824

http://dev.clojure.org/jira/browse/CLJS-1658

http://dev.clojure.org/jira/browse/CLJS-1762

anything else?

this is prep for release because 1824 is critical

@dnolen there are 2 outstanding patches from me: 1- adding 0/1 arity to into (which is still unreleased in Clojure so you might want to hold off) http://dev.clojure.org/jira/browse/CLJS-1809 2- moving the REPL special docstrings to their actual places now that macros exist (`require` etc) http://dev.clojure.org/jira/browse/CLJS-1814

http://dev.clojure.org/jira/browse/CLJS-1812 and http://dev.clojure.org/jira/browse/CLJS-1808 are also ready for review 🙂

@anmonteiro @jrheard ok!

awesome, thanks!

@anmonteiro are the new macros for require...bootstrap compatible as well by any chance?

@richiardiandrea they are!

I’ve been battle-testing them in bootstrap in a thing, and they work great

Awesome! Replumb has been waiting for that for long time actually @anmonteiro this is great. I will replace them and run the tests in order to provide additional validation ;)

great!

@jrheard I’m going to have to pass on those tickets sorry - you should really avoid touching anything except what’s involved in patch

it makes reviewing patches extremely unpleasant

k, good to know, will try again 🙂

i think on CLJS-1808 patch 1 should just do what you want

http://dev.clojure.org/jira/secure/attachment/16006/patch-0001.patch is just a one-line documentation change, avoids touching anything else

@jrheard does that copy Clojure?

the former version of the code copies clojure, but is incorrect because cljs.spec.test uses a different namespace for this keyword (cljs.test.check rather than cljs.spec.test.check)

clojure: https://github.com/clojure/clojure/blob/master/src/clj/clojure/spec/test.clj#L19

cljs: https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/spec/test.cljs#L19

@robert-stuttaford you’re going to have explain the rationale behind 1821

@dnolen I helped him with that patch. the issue there is that behavior becomes non-deterministic because add-js-sources doesn’t return them in dependency order

@thheller http://dev.clojure.org/jira/secure/attachment/15720/implements-false-positives-with-sentinel.patch needs a rebase and 2 nits

PROTOCOL_SENTINEL, and don’t use js*

@anmonteiro doesn’t return what in dependency order?

more, there’s no dependency re-ordering after add-preloads is run

the JS sources

sorting preloads in dep order is not something we care about

it’s not preloads, it’s the foreign libs (JS sources)

@anmonteiro After add-js-sources all Cljs ns are already compiled so does the ordering matter?

@anmonteiro can you explain further?

what does foreigns deps have to do with preloads?

like a preload depends on a foreign-dep?

no that’s not what the problem was. let me think it through again

if this is the case I want to understand the usecase here before saying yes to this one

preloads is supposed be a small thing for dev time

not something to be abused

this has nothing to do with preloads

ah k

hmm looking at the whole add-preloads... hmm

the problem Robert was having was with Closure modules

@dnolen so the JS sources need to be ordered so that modules get output with the correct ordering of JS sources

I think that is the actual underlying issue. I hope I’m explaining myself correctly

@anmonteiro so the issue title is misleading?

@dnolen was using js* since js-obj is not available yet

we specifically bisected the issue to the preloads commit

@thheller why not #js {} ?

yea, the title might be misleading

I hope I’m giving enough context

ah hehe right that should do it too

bit busy right now, will do the rebase tomorrow

@thheller no worries, then I’ll handle it - just wanted to give you credit if had time for it

no worries, go for it

thanks much

@anmonteiro lets fix the title of the issue and I will look at it again with your information in mind

Does add-preloads require that inputs are already sorted? (If yes, why?) To avoud unncessary dep sorting, it would be possible to just move add-preloads call before dependency-order call in build

@juhoteperi we’re going to leave preloads alone for now 🙂 since it’s not related to this issue anyhow

Okay.

@juhoteperi can we rebase 1762?

@dnolen what should I change the issue title to? add-preloads breaks the ordering required by Closure modules?

@anmonteiro that’s better yeah

done

the commit message still has the old title though

@anmonteiro preferable to fix that too

it’s Robert’s patch, though. can I fix it without stealing credit?

@anmonteiro @juhoteperi but I’m now questioning this ticket entirely

I don’t why any of this matters

you should NOT be using :preloads in production

if someone is doing that, I don’t think I care

@dnolen then we should fix add-preloads to only ever touch sources if :preloads is specified

@anmonteiro definitely

because it breaks the ordering even if :preloads is not specified

so there’s the bug

that’s right

definitely makes more sense to do that

@anmonteiro lets also add a warning about :preloads for any optimization setting other than :none please

to discourage :preloads abuse

definitely

I’ll put a new patch together for that issue

@dnolen re: cljs-1672 rebase, sure

@juhoteperi thanks!

@anmonteiro thanks!

should we just create a new issue entirely?

and reject cljs-1821

@anmonteiro nah, lets just make the ticket reflect the problem now that we understand it

gotcha

@dnolen changing the title again 🙂

heh thanks

@dnolen what’s the preferred way to add a warning in cljs.closure?

this doesn’t seem to warrant a new analyzer warning

should we just hard-error with an assert like we do in the other opts validation functions?

By the way, I think quite many are probably using :preloads to enable cljs-devtools even in advanced builds

@juhoteperi too bad

(myself included, in some projects)

@anmonteiro warning is fine

I have zero interest in developing :preloads at all

cljs-devtools don’t work in advanced builds, but nothing is stopping people to leave them in preloads I guess

and a warning means people can stop doing that gradually

Yeah.

everything I said about :preloads has been qualified by dev time

@darwin console formatters work, at least to some degree, though they are quite useless as most of objects anyway have mangled properties

@dnolen: my question still holds, how should we warn in cljs.closure? Print to stderr?

@anmonteiro yes

Gotcha, thanks

@juhoteperi: hmm interesting, several months ago I tested it under advanced mode, and this test would fail: https://github.com/binaryage/cljs-devtools/blob/master/src/lib/devtools/formatters/helpers.cljs#L42 maybe your cljs types have some formatting protocols? https://github.com/binaryage/cljs-devtools/blob/master/src/lib/devtools/formatters/helpers.cljs#L29

Huh. Not sure. Anyway, I don't need console formatting with advanced builds, just something I noticed when I had accidentally had cljs-devtools enabled on adv build.

@dnolen just attached CLJS-1821-2.patch: http://dev.clojure.org/jira/browse/CLJS-1821

@anmonteiro thanks

@dnolen just found a regression in ClojureScript master wrt. the source-map option

please hold off on the release, fixing it now

specifically because of this commit: https://github.com/clojure/clojurescript/commit/ab7a4911f1fd3a81210b1a9f2d84857748f8268b

@dnolen OK fixed in http://dev.clojure.org/jira/browse/CLJS-1825

@anmonteiro thanks applied

@dnolen cool. now that I got you here, I think I found a bug in self-hosted where we don’t honor :reload and reload-all when requiring. this would be the fix: https://github.com/anmonteiro/clojurescript/commit/14f67e2724a61357431f986192da59cb52aaaca2

I’ll attach it to a JIRA ticket, but wanted confirmation that it’s indeed a bug

@anmonteiro looks OK to me

@dnolen cool, creating a ticket + patch

@dnolen http://dev.clojure.org/jira/browse/CLJS-1826

so much goodness coming in this next version 💯

@juhoteperi let me know when you get a chance to rebase, it’s the only other thing I want to get into this release

Ah, doing now!

@dnolen Did you have any specific need for rebase? I just checked that it applies cleanly.

(http://dev.clojure.org/jira/secure/attachment/16027/CLJS-1762-4.patch this one)

@juhoteperi oops, looks like my tree was dirty - yes it applies

ok now would be a good time to test master 🙂

Great!

will probably release first thing tomorrow

thanks everyone!

@dnolen FWIW I’ve been running master ever since the “require without NS” commit

sweet

Tested with Boot-cljs both none and adv and working fine

And warning about :preloads with advanced build worked 🙂