Fork me on GitHub
#cljs-dev
<
2016-10-18
>
Yehonathan Sharvit13:10:26

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

Yehonathan Sharvit13:10:00

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

Yehonathan Sharvit13:10:10

It might worth documenting it

dnolen13:10:46

@viebel there’s no issue

dnolen13:10:04

async stuff is async

dnolen13:10:07

bindings aren’t going to work

Yehonathan Sharvit13:10:39

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

dnolen13:10:44

@viebel this is not a problem

Yehonathan Sharvit13:10:56

This is why I think we need to document it

dnolen13:10:56

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

dnolen13:10:19

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

Yehonathan Sharvit13:10:14

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*

dnolen13:10:01

to be honest I’m not that interested

dnolen13:10:13

talking about core.async is of course irrelevant

dnolen13:10:48

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

dnolen13:10:22

simply enumerating the bindings established by the fns in that namespace

dnolen13:10:55

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

dnolen13:10:09

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

dnolen13:10:40

the right place to cover this is probably binding

Yehonathan Sharvit13:10:13

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

dnolen14:10:35

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

dnolen15:10:57

anything else?

dnolen15:10:10

this is prep for release because 1824 is critical

anmonteiro15:10:46

@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

anmonteiro15:10:12

awesome, thanks!

richiardiandrea16:10:09

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

anmonteiro16:10:44

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

richiardiandrea16:10:52

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 ;)

dnolen16:10:54

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

dnolen16:10:04

it makes reviewing patches extremely unpleasant

jrheard16:10:14

k, good to know, will try again 🙂

jrheard16:10:22

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

jrheard16:10:40

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

dnolen16:10:17

@jrheard does that copy Clojure?

jrheard16:10:05

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)

dnolen16:10:53

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

anmonteiro16:10:51

@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

dnolen16:10:06

PROTOCOL_SENTINEL, and don’t use js*

dnolen16:10:59

@anmonteiro doesn’t return what in dependency order?

anmonteiro16:10:03

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

anmonteiro16:10:06

the JS sources

dnolen16:10:12

sorting preloads in dep order is not something we care about

anmonteiro16:10:24

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

juhoteperi16:10:29

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

dnolen16:10:04

@anmonteiro can you explain further?

dnolen16:10:11

what does foreigns deps have to do with preloads?

dnolen16:10:16

like a preload depends on a foreign-dep?

anmonteiro16:10:35

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

dnolen16:10:37

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

dnolen16:10:56

preloads is supposed be a small thing for dev time

dnolen16:10:00

not something to be abused

anmonteiro16:10:01

this has nothing to do with preloads

juhoteperi16:10:18

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

anmonteiro16:10:26

the problem Robert was having was with Closure modules

anmonteiro16:10:48

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

anmonteiro16:10:19

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

dnolen16:10:28

@anmonteiro so the issue title is misleading?

thheller16:10:41

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

anmonteiro16:10:41

we specifically bisected the issue to the preloads commit

dnolen16:10:50

@thheller why not #js {} ?

anmonteiro16:10:51

yea, the title might be misleading

anmonteiro16:10:59

I hope I’m giving enough context

thheller16:10:04

ah hehe right that should do it too

thheller16:10:32

bit busy right now, will do the rebase tomorrow

dnolen16:10:51

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

thheller16:10:03

no worries, go for it

dnolen16:10:07

thanks much

dnolen16:10:33

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

juhoteperi16:10:01

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

dnolen16:10:36

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

dnolen16:10:03

@juhoteperi can we rebase 1762?

anmonteiro16:10:28

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

dnolen16:10:38

@anmonteiro that’s better yeah

anmonteiro16:10:12

the commit message still has the old title though

dnolen16:10:31

@anmonteiro preferable to fix that too

anmonteiro16:10:46

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

dnolen16:10:52

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

dnolen16:10:58

I don’t why any of this matters

dnolen16:10:04

you should NOT be using :preloads in production

dnolen16:10:08

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

anmonteiro16:10:44

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

anmonteiro16:10:01

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

dnolen16:10:08

so there’s the bug

anmonteiro16:10:14

that’s right

anmonteiro16:10:24

definitely makes more sense to do that

dnolen16:10:39

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

dnolen16:10:45

to discourage :preloads abuse

anmonteiro16:10:19

I’ll put a new patch together for that issue

juhoteperi16:10:33

@dnolen re: cljs-1672 rebase, sure

anmonteiro16:10:15

should we just create a new issue entirely?

anmonteiro16:10:29

and reject cljs-1821

dnolen16:10:37

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

anmonteiro16:10:49

@dnolen changing the title again 🙂

dnolen16:10:53

heh thanks

anmonteiro16:10:41

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

anmonteiro16:10:50

this doesn’t seem to warrant a new analyzer warning

anmonteiro16:10:40

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

juhoteperi17:10:54

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

juhoteperi17:10:31

(myself included, in some projects)

dnolen17:10:59

I have zero interest in developing :preloads at all

darwin17:10:14

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

dnolen17:10:28

and a warning means people can stop doing that gradually

dnolen17:10:57

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

juhoteperi17:10:35

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

anmonteiro17:10:47

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

anmonteiro17:10:05

Gotcha, thanks

darwin17:10:23

@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

juhoteperi17:10:31

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.

anmonteiro18:10:25

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

anmonteiro18:10:35

please hold off on the release, fixing it now

anmonteiro18:10:12

@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

anmonteiro18:10:47

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

anmonteiro18:10:40

@dnolen cool, creating a ticket + patch

anmonteiro19:10:34

so much goodness coming in this next version cljs 💯

dnolen21:10:32

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

juhoteperi21:10:57

Ah, doing now!

juhoteperi21:10:02

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

dnolen21:10:41

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

dnolen21:10:01

ok now would be a good time to test master 🙂

dnolen21:10:08

will probably release first thing tomorrow

dnolen21:10:16

thanks everyone!

anmonteiro21:10:16

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

juhoteperi21:10:21

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

juhoteperi21:10:10

And warning about :preloads with advanced build worked 🙂