Fork me on GitHub
#cljs-dev
<
2017-07-08
>
martinklepsch10:07:34

I think there’s an issue with {:libs [,,,]} style JS dependencies

martinklepsch10:07:20

Introduced in 9e5b7ac64dd0388d3403f48381c80ba1e3998da8 but just reverting that one on master doesn’t fix it

martinklepsch11:07:19

ok, correction, cljsjs/incremental-dom seems to be the sole offender whereas e.g. openlayers works as expected

mfikes13:07:03

It is trivial to get pre-built ChakraCore binaries and have our unit tests run over them (they run in about 2 seconds). Willing to contribute patches if there is a desire to make it a thing. Here is a high-level gist: https://gist.github.com/mfikes/b675baa14946e11968a3f191ee49f932

juhoteperi14:07:42

I haven't yet had time to properly test if/how string based requires work with non-js-module foreign-libs, but I think it would be important to support those also, to provide way for libraries to depend on js-libs no matter how the js-lib is provided (npm-deps, cljsjs package)

juhoteperi14:07:20

But that can also be added later

juhoteperi14:07:24

But it is great weather today so testing this and writing the blog post will have to wait a bit

dnolen14:07:43

ha, enjoy! 🙂

dnolen15:07:46

I’ll finish up CLJS-2163

mfikes15:07:06

@dnolen Yes, CLJS-1644 still occurs on master. In fact, a duplicate of it was recently written, which I have now closed: https://dev.clojure.org/jira/browse/CLJS-2129

dnolen15:07:18

k thanks noted that on the ticket so I don’t forget next time

dnolen16:07:28

k just finished another big JIRA sweep

anmonteiro17:07:02

@dnolen I tested CLJS-1764 the other day and it's still a problem

anmonteiro17:07:14

I think you closed it this morning

anmonteiro17:07:26

(Double warning at the REPL)

anmonteiro17:07:50

IIRC the minimal repro doesn't even contain function (bodies)

dnolen17:07:52

I only get one warning for

dnolen17:07:00

(defn foo [] x)

dnolen17:07:15

and (def foo x)

dnolen17:07:50

@anmonteiro I’m also having trouble again with :npm-deps on a different machine https://dev.clojure.org/jira/browse/CLJS-2193

dnolen17:07:20

we actually have a test case now which checks that there’s only one

dnolen17:07:29

(re: double warning)

anmonteiro17:07:32

java -cp cljs.jar:esrc clojure.main -m cljs.repl.node
ClojureScript Node.js REPL server listening on 49560
To quit, type: :cljs/quit
cljs.user=> *clojurescript-version*
"1.9.730"
cljs.user=> (require 'foo.core)
WARNING: Use of undeclared Var foo.core/abc at line 5 /Users/anmonteiro/Documents/github/clojurescript/esrc/foo/core.cljs
WARNING: Use of undeclared Var foo.core/abc at line 5 /Users/anmonteiro/Documents/github/clojurescript/esrc/foo/core.cljs
true
cljs.user=>

anmonteiro17:07:58

@dnolen ^ using the exact repro in the ticket

dnolen17:07:43

@anmonteiro oh and you supplied patch for this (I see now REPL only)

dnolen17:07:13

sorry read through comments and got confused

anmonteiro17:07:15

yeah but I remember you didn’t like the patch for some reason

dnolen17:07:53

because of the ticket description

dnolen17:07:06

was impossible to tell if REPL only or part of the other problems

dnolen17:07:40

oh but ok yeah I do not understand the patch

anmonteiro17:07:41

here’s what I remember: the REPL ends up analyzing namespaces twice in cljs.repl/load-namespace

anmonteiro17:07:13

cljs.closure/cljs-dependencies was the place where the 2nd analysis would be made IIRC

anmonteiro17:07:25

I can have another proper look today to see if we can solve it better

dnolen17:07:19

@anmonteiro OK, thanks - I re-opened and left feedback on the patch

dnolen17:07:31

I would say that this is not high-priority though since REPL thing

anmonteiro17:07:41

@dnolen just saw CLJS-2193 too. I think I know what’s going on

anmonteiro17:07:01

I updated module_deps.js but didn’t update the packages we install 🙂

anmonteiro17:07:28

I think we should still keep installing these implicitly

dnolen17:07:41

yes they are required

anmonteiro17:07:45

just as long as we install them… which we’re not doing

anmonteiro17:07:52

thankfully we have tests for it now

dnolen17:07:07

I don’t want to actually expose how we do this

dnolen17:07:12

(i.e. tell users what to install)

anmonteiro17:07:13

how do I repro?

dnolen17:07:37

I think you need to blow away node_modules

dnolen17:07:46

and I think you need to make sure you don’t have those deps globally installed

anmonteiro17:07:10

that’s pretty hard to do since module-deps and resolve are transitive dependencies of NPM!

anmonteiro17:07:23

running npm 5 here

anmonteiro17:07:35

this makes much more sense now

dnolen17:07:42

oh is that a separate install from Node.js ???

anmonteiro17:07:55

Node 8 comes with NPM5 now

anmonteiro17:07:13

I think there’s one way to not rely on globally installed modules

anmonteiro17:07:24

export NODE_PATH=

dnolen17:07:28

hold on …

dnolen17:07:44

yeah I guess we don’t want to restrict to Node 8

dnolen17:07:50

lot of people would get cut off

anmonteiro17:07:58

hrm yeah I can’t repro locally

anmonteiro17:07:01

going to install Node 6

anmonteiro17:07:28

reproed with node 6 / npm 4

anmonteiro17:07:24

also submitted an update to the ClojureScript website: https://github.com/clojure/clojurescript-site/pull/108

dnolen17:07:43

cool thanks

anmonteiro17:07:15

@dnolen is the current directory implicitly added to the classpath in Java (or maybe Lein)?

anmonteiro17:07:55

just spent some time trying to figure out why my fix wasn’t working, I had a compiled CLJS uberjar in my clojurescript directory

dnolen17:07:52

yeah you have to watch out for that

dnolen17:07:01

target is on the classpath, I always remove it before testing

anmonteiro17:07:14

hah, maybe that

anmonteiro17:07:46

OK patch attached to CLJS-2193

dnolen18:07:45

@bronsa are you aware of this tools.reader (edn/read-string ";foo") fails w/ EOF

dnolen18:07:57

(I’m in the middle of trying to switch cljs.reader to tools.reader)

bronsa18:07:49

no, the correct behaviour should be that tools.reader fails w/ EOF but tools.reader.edn doesn't (to mimic clojure)

bronsa18:07:07

if that's not the case in the cljs port it's a bug, open a ticket & I'll take a look

dnolen18:07:03

@bronsa that exact line works in Clojure

bronsa18:07:01

I'll take a look

bronsa18:07:28

I can't repro

bronsa18:07:40

cljs.user=> (require '[clojure.tools.reader.edn :as e])

cljs.user=> (e/read-string "")
nil
cljs.user=> (e/read-string ";foo")
nil

bronsa18:07:50

this is on 1.0.0

bronsa18:07:43

to be clear, clojure.tools.reader.edn/read-string returns nil on EOF, while clojure.tools.reader/read-string throws

bronsa18:07:03

this is to parallel clojure.core/read-string that throws while clojure.edn/read-string doesn't

bronsa18:07:31

if you want clojure.tools.reader/read-string to return nil, you'll have to be explicit about it

bronsa18:07:43

and do (r/read-string {:eof nil} ";foo")

dnolen18:07:38

@bronsa cljs.tools.reader.edn

dnolen18:07:59

I’m talking about replacing cljs.reader the runtime reader

bronsa18:07:19

yes I understand that, I'm saying that I can't reproduce your EOF using the .edn reader, see

[~/src/tools.reader]> lein trampoline cljsbuild repl-rhino
Running Rhino-based ClojureScript REPL.
To quit, type: :cljs/quit
cljs.user=> (require '[cljs.tools.reader.edn :as e])

cljs.user=> (e/read-string ";foo")
nil

bronsa18:07:50

if you see something different than this that might be due to different js environments but you'll have to tell me how to repro

dnolen18:07:01

@bronsa no this is enough, I made a mistake

dnolen18:07:07

what is eof supposed to be by default?

dnolen18:07:35

I’m passing nil

bronsa18:07:43

tools.reader follows the same API as clojure, so it's an EOF exception for the clojure reader and nil for the edn reader

bronsa18:07:53

nil is correct for edn

bronsa18:07:30

(beware that if you use the 2-arity version you'll have to pass :eof nil explicitly in the opts map)

dnolen18:07:41

that’s what I missed

bronsa18:07:10

yeah, not the best API but I wanted 1:1 with clojure and that's what clojure.edn does

dnolen18:07:16

@bronsa totally understood thanks

dnolen18:07:19

now maybe a real bug

dnolen18:07:32

(reader/read-string "'bar")

dnolen18:07:42

returns a symbol constructed in a strange way

dnolen18:07:50

(symbol nil "'bar")

dnolen18:07:07

can be seen when you try this

dnolen18:07:16

(= 'bar (reader/read-string "'bar"))

bronsa18:07:29

that's expected behaviour

bronsa18:07:41

the edn reader doesn't implement quote

bronsa18:07:18

and IIRC ' is a valid symbol constituent char in EDN

bronsa18:07:19

so if you read 'foo as EDN, you'll get the 'foo symbol

bronsa18:07:52

(if ' is not in the EDN spec, then we should throw instead, but clojure.edn behaves like that so tools.reader has at least parity with the official reader behaviour)

dnolen18:07:25

finding lots of bad reader tests 🙂

bronsa18:07:49

while we're talking about reader tests, are any of those dependant from the error message being thrown?

dnolen18:07:00

@bronsa is it not possible to set the default-reader-fn with edn reader?

dnolen18:07:22

@bronsa no we don’t test for thrown error strings

dnolen18:07:36

(except for one case which isn’t relevant anymore)

bronsa18:07:42

ok cool, context is I'm looking to cut a beta with https://dev.clojure.org/jira/browse/TRDR-44 next week so the error messages will change

bronsa18:07:51

what's default-reader-fn?

anmonteiro18:07:09

@bronsa quick q: for reading input strings at a REPL should I use c.t.r.edn?

anmonteiro18:07:27

or rather, to make a REPL

anmonteiro18:07:30

Lumo in this case

bronsa18:07:38

c.t.r is for code, c.t.r.edn is for EDN data

bronsa18:07:51

the latter doesn't implement stuff like quote, syntax-quote, var, etc

bronsa18:07:55

which you need for code

dnolen18:07:22

@bronsa *default-data-reader-fn* sorry

dnolen18:07:29

the handler for unknown tags

bronsa18:07:40

yeah, not through that api

bronsa18:07:55

{:readers {tag -> fn}} in the opts map

bronsa18:07:23

and then {:default default-fn} for the default data reader

bronsa18:07:14

clojure.tools.reader.edn> (read-string {:readers {'foo (constantly 1)}
                                        :default (constantly 2)}
                                       "#foo 123")
1
clojure.tools.reader.edn> (read-string {:readers {'foo (constantly 1)}
                                        :default (constantly 2)}
                                       "#bar 123")
2

bronsa18:07:58

seems like cljs.reader is a mix of the clojure reader and clojure.edn in terms of API?

dnolen18:07:24

yes it’s OK

dnolen18:07:28

preserving the old api isn’t hard

anmonteiro18:07:47

@dnolen another cljs.util/relative-name bug.. added tests for this case https://dev.clojure.org/jira/browse/CLJS-2194

dnolen18:07:28

@bronsa is TRDR-44 the array map thing?

anmonteiro18:07:29

found while fixing CLJS-2152

dnolen18:07:38

k will look

bronsa18:07:42

no, that's already in 1.0 IIRC,

bronsa18:07:57

TRDR-44 is about more informative & more consistent error messages

bronsa18:07:49

yeah the array-map one was TRDR-40 and is in 1.0 (unless there are other array-map issues i'm not aware of)

dnolen18:07:14

(type (reader/read-string "{:a 1 :b 2 :c 3}")) doesn’t return what I would expect

dnolen18:07:23

ClojureScript relies on 1.0.0

bronsa18:07:58

I think the fix didn't go in for the .edn segment

bronsa18:07:53

yeah, will port now

dnolen19:07:13

probably could use some code review when people have time https://github.com/clojure/clojurescript/pull/69/files

dnolen19:07:19

but this basically seems to be it

bronsa19:07:20

fix is on master

bronsa19:07:35

can cut 1.0.1 if you want

dnolen19:07:51

@bronsa that would be great thanks

anmonteiro19:07:48

wrapping up the fix to CLJS-2152

anmonteiro19:07:13

oh npm-deps-test fail

anmonteiro19:07:19

so I guess apply 2193 before 🙂

dnolen19:07:34

yes done 🙂

dnolen19:07:47

gotta run but will be back later to push stuff along

bronsa19:07:56

is (.fromArray PersistentHashMap ..) fine or should I be using (.fromArray cljs.core/PersistentHashMap ..) instead?

anmonteiro19:07:03

so deferring to tools.reader from cljs.reader fixes CLJS-2059 which is also on the blockers list 👍

anmonteiro19:07:07

(reading namespaced maps)

anmonteiro19:07:12

just confirmed locally

anmonteiro19:07:59

which means CLJS-1959 is now the only blocker for the next release 🎉

bronsa19:07:59

when are you guys planning to cut a release?

anmonteiro19:07:21

I think David mentioned before EuroClojure

anmonteiro19:07:31

Which is in 2 weeks

anmonteiro19:07:46

Might be this week or the next I believe

bronsa19:07:31

@dnolen PR looks fine to me, only minor thing is that the docstrings you copied over refer to clojure.tools.reader.edn vars, not sure if you want to change that or make the relationship between cljs.reader and cljs.tools.reader.edn more explicit

anmonteiro20:07:35

@dnolen the reader PR looks good with the exception of: - 1 failing test in pprint-test - 1 failing test in tagged-literals test

anmonteiro20:07:16

the 2nd one is easily fixed, left an inline comment

anmonteiro20:07:32

the first one fails on (reader/read-string "@@(ref (ref 1))")

anmonteiro20:07:43

@bronsa ^ is this valid under c.t.r.edn?

bronsa20:07:04

deref is not EDN

anmonteiro20:07:09

right, so that’s probably why the test is failing 🙂

anmonteiro20:07:21

maybe it’s a bad test

bronsa20:07:33

looks likely

anmonteiro20:07:54

(it’s in the nice-to-haves list)

dnolen20:07:14

@bronsa either should be fine, re: .fromArray

dnolen20:07:20

@bronsa actually I’m wrong

dnolen20:07:32

we added .createWithCheck

bronsa20:07:26

how does that work?

bronsa20:07:37

we're currently doing

(if (<= map-count (* 2 (.-HASHMAP-THRESHOLD cljs.core/PersistentArrayMap)))
        (.fromArray cljs.core/PersistentArrayMap (to-array the-map) true true)
        (.fromArray cljs.core/PersistentHashMap (to-array the-map) true))

dnolen20:07:21

@bronsa it’s the only one that will do the checks across PAM & PHM, no-clone parameter is not useful since we have to traverse anyway

dnolen20:07:10

it was added 6 months ago - so it’ll be an incompability with older versions of ClojureScript

bronsa21:07:49

I don't know how many people are using t.r in cljs but I'd rather not break backward compatibility if it's not strictly necessary

dnolen21:07:03

I don’t really see how you can avoid it if you want the correct behavior though

dnolen21:07:15

there’s no other hook for you that actually checks

bronsa21:07:29

is the check just the duplicate keys check?

dnolen21:07:33

that’s right

bronsa21:07:42

ok, so that's not really an issue, we're doing it at read-time

dnolen21:07:02

I wasn’t sure you did it at read time

mfikes21:07:01

The ChakraCore testing effort perhaps paid off in a sense: While things pass on macOS and Linux, there are three failures on Windows. I don’t know if they are specific to ChakraCore or Windows. If I could figure out how to copy text from the window (grr), I’d share them. Lacking that here is a picture of showing the 3 failures https://twitter.com/mfikes/status/883799875858694144/photo/1

dnolen21:07:10

yeah let’s get that tweak to the test script in

dnolen21:07:25

@anmonteiro one weird thing I noticed is that lein-test doesn’t seem idempotent wrt. to npm-deps-test

mfikes21:07:33

Yeah, the patches are ready, and can be applied (they don’t cause anything to break in CI or on macOS)

dnolen21:07:43

the second test run always reports JSDoc warnings

anmonteiro21:07:02

hrm, let me try that

dnolen21:07:37

@mfikes it’s a bit strange that the ChakraCore binary doesn’t ship a REPL

anmonteiro21:07:00

I think there’s ChakraNode

mfikes21:07:14

There is a package.json file or somesuch that is written to the filesystem when running the tests. Perhaps that leads to lack of idempotency?

anmonteiro21:07:29

hah, that could be it actually

anmonteiro21:07:41

maybe we need to wipe that between tests

anmonteiro21:07:51

I’ll try to repro

mfikes21:07:59

Yes, there is ChakraNode—I had used it to make a ClojureScript REPL previously. But David is right, the ch command insists on a file being fed to it rather than it dumping you into a REPL.

dnolen21:07:11

@bronsa (reader/read-string "#{:a :b :c :d :a}") doesn’t throw, expected?

bronsa21:07:01

it's fixed in the better errors branch I'm working at

bronsa21:07:14

hope to get that merged tomorrow/early next week

dnolen21:07:06

merged cljs-1800 branch, will bump tools.reader when you release @bronsa - thanks for the help

bronsa21:07:44

1.0.1 with the array-map fix is already out

bronsa21:07:08

1.0.2 with duplicate checks on sets & better errors will be out in a few days

mfikes21:07:29

FWIW, SpiderMonkey says “no” on Windows with:

TypeError: malformed UTF-8 character sequence at offset 887730
if you try to do js.exe -f core-advanced-tests.js. Maybe we can catch this kind of stuff with that Windows-based CI setup António mentioned.

dnolen22:07:42

interesting

dnolen22:07:24

@anmonteiro I’m starting to feel somewhat inclined to roll back the changes to boostrap.js for CLJS-1959

anmonteiro22:07:40

I think that would fix it

dnolen22:07:40

it’s super convenient to be able to test stuff with Node.js - but perhaps not actually important

anmonteiro22:07:50

@dnolen what would we lose?

dnolen22:07:19

you can’t expect :npm-deps stuff to work under Node.js

dnolen22:07:27

that and the other one where we patch dirname & filename

dnolen22:07:33

basically what bootstrap.js was in 2016

dnolen22:07:12

@anmonteiro I’d take a patch to take us back to this

dnolen22:07:21

should do a round of testing

dnolen22:07:36

but this should fix Node.js target users

dnolen22:07:54

people using :npm-deps should just be testing in the browser

dnolen22:07:06

gotta run again but that’s what I’m leaning toward - it’s just not worth it to make Node.js users lifes more complicated to change our bootstrap.js scriopt

mfikes22:07:30

I’ve isolated the Windows issues I mentioned above to bad JavaScript being generated when compiling on Windows. (The same errors appear when taking the JavaScript generated there and running in the interpreters on macOS, and no errors appear when taking JavaScript generated on macOS and running them in the Windows interpreters.) The malformed Unicode that SpiderMonkey is not happy with is probably related to low-level Closure. It also occurs in :simple mode, and the error is right at the á in this line:

goog.string.FORCE_NON_DOM_HTML_UNESCAPING=!1;goog.string.Unicode={NBSP:"á"};
Good news is, if you develop on macOS or Linux, the code emitted is good. I suspect you run into issues if you try to produce production code on a Windows box.

anmonteiro23:07:51

submitted a patch in https://dev.clojure.org/jira/browse/CLJS-2195 making NPM deps tests idempotent