This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2016-10-12
Channels
- # beginners (34)
- # boot (210)
- # cider (16)
- # cljs-dev (65)
- # cljsrn (3)
- # clojars (2)
- # clojure (107)
- # clojure-austin (8)
- # clojure-berlin (10)
- # clojure-brasil (1)
- # clojure-canada (1)
- # clojure-dev (1)
- # clojure-fr (1)
- # clojure-italy (22)
- # clojure-new-zealand (12)
- # clojure-nl (28)
- # clojure-russia (13)
- # clojure-spec (25)
- # clojure-uk (10)
- # clojurescript (109)
- # cursive (18)
- # datomic (44)
- # defnpodcast (1)
- # dirac (4)
- # emacs (2)
- # funcool (1)
- # hoplon (16)
- # jobs (14)
- # lambdaisland (23)
- # leiningen (2)
- # luminus (3)
- # off-topic (7)
- # om (58)
- # onyx (16)
- # proton (6)
- # re-frame (42)
- # reagent (55)
- # ring-swagger (5)
- # untangled (47)
- # vim (9)
@dnolen are you looking into CLJS-1817 or can I assign it to myself?
I saw you made some changes to the description
@anmonteiro I was looking at it a bit just now but if you want to take it go for it
@dnolen I’ve got a fix I think
something that was incorrectly ported from Clojure’s data structures
running the tests now
whatever test we create for CLJS-1817 should probably careful to be designed around hash collisions
@dnolen ups just saw this after attaching the patch
anyway, there’s a patch now in http://dev.clojure.org/jira/browse/CLJS-1817
the commit message includes the explanation for the fix
let me know if you want me to change the test
@anmonteiro so the problem is if we fix CLJS-1818 that test isn’t doing anything 🙂
yeah that’s right
@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?
I can probably use with-redefs
to test the collision case otherwise
@anmonteiro that seems ok to me
@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}
@anmonteiro yes we need key dupe checks for array-map
@dnolen OK updated CLJS-1817’s patch with a proper test for hash collisions
btw the hash for true
is also wrong wrt. Clojure's
related to CLJS-1818
hrm, so went ahead and fixed CLJS-1818 too
the patch mirrors JVM’s behavior. link included in the commit message
I finally got around to going through fixme notes on Closure update: https://github.com/clojure/clojurescript/compare/master...Deraen:cljs-1762-4
I fixed process-js-modules
so that it processes modules from both :foreign-libs
and :ups-foreign-libs
at the same time
@juhoteperi do you have a diff from the previous diff to this one?
i.e. not the whole thing since you started working on this
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
Looks like github is not able to show the diff
no problem
But Git itself can do it 🙂
oh great! makes easier to see this last step, thanks!
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.
But maybe that is not necessary for now. The one test I wrote is at least better than nothing.
@juhoteperi yeah the circle color test bit isn’t critical
Hmm, one thing I was just thinking: process-js-modules should probably be wrapped in util/measure
^ agreed, especially given the recent http://dev.clojure.org/jira/browse/CLJS-1816
@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
we might want to log that in JIRA
Not sure how important that is
yeah, I don’t know how many stuff relies on AMD modules currently
I expect it to be lower and lower over time as well, esp. given ES6
Process JS modules, elapsed time: 2424.324872 msecs @ circle-color
Process JS modules, elapsed time: 2.327219 msecs @ project without module processing
hrm isn’t there a way to omit the log message if we don’t process any modules?
And around 0.05 msecs on incremental compile
Yeah, but does it matter? These messages are for debugging anyway
And this shows that process-modules doesn't accidentally do some work
right. on the other hand it might confuse people because a message is showing for something they don’t have in their project
Okay, done: https://github.com/clojure/clojurescript/commit/7aa1be9c7d81699cbdbe22f7ea509e40f5c2efee?diff=unified
New patch attached on Jira.
I think this should be ready for merge.