Fork me on GitHub
#cljs-dev
<
2016-10-12
>
anmonteiro12:10:04

@dnolen are you looking into CLJS-1817 or can I assign it to myself?

anmonteiro12:10:12

I saw you made some changes to the description

dnolen12:10:32

@anmonteiro I was looking at it a bit just now but if you want to take it go for it

anmonteiro12:10:40

@dnolen I’ve got a fix I think

dnolen12:10:54

it’s definitely strange - only PersistentHashMap

anmonteiro12:10:57

something that was incorrectly ported from Clojure’s data structures

anmonteiro12:10:09

running the tests now

dnolen12:10:01

whatever test we create for CLJS-1817 should probably careful to be designed around hash collisions

dnolen12:10:15

not just the particular example given

anmonteiro12:10:33

@dnolen ups just saw this after attaching the patch

anmonteiro12:10:08

the commit message includes the explanation for the fix

anmonteiro12:10:13

let me know if you want me to change the test

dnolen12:10:58

@anmonteiro so the problem is if we fix CLJS-1818 that test isn’t doing anything 🙂

anmonteiro12:10:18

yeah that’s right

anmonteiro12:10:07

@dnolen I’ll fix the test to account for a collision. do you have any quick example of different stuff that hashes to the same thing?

anmonteiro12:10:23

I can probably use with-redefs to test the collision case otherwise

dnolen12:10:59

@anmonteiro that seems ok to me

dnolen12:10:16

otherwise we’d have to search for collisions

anmonteiro12:10:19

@dnolen btw this seems wrong to me:

cljs.user=> (array-map '0 1 0 0)
{0 1, 0 0}
cljs.user=> (hash-map '0 1 0 0)
{0 0}

dnolen12:10:20

and hard wire them in - brittle

dnolen12:10:25

or find them - slow

dnolen12:10:46

@anmonteiro yes we need key dupe checks for array-map

anmonteiro13:10:57

@dnolen OK updated CLJS-1817’s patch with a proper test for hash collisions

dnolen13:10:07

great thanks

anmonteiro13:10:43

btw the hash for true is also wrong wrt. Clojure's

anmonteiro13:10:50

related to CLJS-1818

anmonteiro13:10:46

hrm, so went ahead and fixed CLJS-1818 too

anmonteiro13:10:01

the patch mirrors JVM’s behavior. link included in the commit message

juhoteperi19:10:50

I finally got around to going through fixme notes on Closure update: https://github.com/clojure/clojurescript/compare/master...Deraen:cljs-1762-4

juhoteperi19:10:19

I fixed process-js-modules so that it processes modules from both :foreign-libs and :ups-foreign-libs at the same time

anmonteiro19:10:40

@juhoteperi do you have a diff from the previous diff to this one?

anmonteiro19:10:48

i.e. not the whole thing since you started working on this

juhoteperi19:10:06

And I decided that perhaps it is okay that amdjs and commonjs modules are processed separately, if this becomes problem we can fix this later

juhoteperi19:10:24

Looks like github is not able to show the diff

juhoteperi19:10:37

But Git itself can do it 🙂

anmonteiro19:10:55

oh great! makes easier to see this last step, thanks!

juhoteperi19:10:08

I was at some point thinking about improving the tests, I think David suggested downloading files for circle-color in tests and using that as test suite.

juhoteperi19:10:05

But maybe that is not necessary for now. The one test I wrote is at least better than nothing.

dnolen19:10:44

@juhoteperi yeah the circle color test bit isn’t critical

juhoteperi19:10:25

Hmm, one thing I was just thinking: process-js-modules should probably be wrapped in util/measure

anmonteiro19:10:16

@juhoteperi this looks great. I think the only thing missing to do in a future enhancement is to allow dependencies between AMD and CommonJS modules

anmonteiro19:10:21

we might want to log that in JIRA

juhoteperi19:10:46

Not sure how important that is

anmonteiro19:10:14

yeah, I don’t know how many stuff relies on AMD modules currently

anmonteiro19:10:28

I expect it to be lower and lower over time as well, esp. given ES6

juhoteperi19:10:34

Process JS modules, elapsed time: 2424.324872 msecs @ circle-color

juhoteperi19:10:54

Process JS modules, elapsed time: 2.327219 msecs @ project without module processing

anmonteiro19:10:42

hrm isn’t there a way to omit the log message if we don’t process any modules?

juhoteperi19:10:55

And around 0.05 msecs on incremental compile

juhoteperi19:10:14

Yeah, but does it matter? These messages are for debugging anyway

juhoteperi19:10:41

And this shows that process-modules doesn't accidentally do some work

anmonteiro19:10:18

right. on the other hand it might confuse people because a message is showing for something they don’t have in their project

dnolen19:10:06

I would argue that we probably shouldn’t show it if there are no modules in the build

juhoteperi19:10:07

New patch attached on Jira.

juhoteperi19:10:33

I think this should be ready for merge.