Fork me on GitHub
#cljs-dev
<
2017-07-07
>
moxaj12:07:07

I believe the fix for CLJS-2109 may have broken some stuff in the wild, I'm having trouble with cljs-time in particular. Consider this:

(ns foo.bar)

(defrecord X [])

(defprotocol Foo
  (a [this])
  (b [this]))

(extend-protocol Foo
  foo.bar.X ;; foo.bar/X ?
  (a [this] 0)
  (b [this] (a this)))
This throws with No such namespace: foo + Use of undeclared Var foo/bar. The commented variant compiles.

moxaj12:07:49

clojure however accepts foo.bar.X but not foo.bar/X

dnolen12:07:32

well we’re unlikely to roll back CLJS-2109 since that’s just a bug

dnolen12:07:41

as to cljs-time that just looks wrong to me anyhow

moxaj12:07:20

in what respect

moxaj12:07:30

(btw that's my minimal repro, not the actual code)

dnolen12:07:42

foo.bar.X is not a supported thing for namespaces

dnolen12:07:02

if it worked, you just got lucky

moxaj12:07:21

now I wonder if clojure not accepting foo.bar/X is a bug or working as intended

moxaj12:07:32

https://github.com/clojure/clojurescript/wiki/Differences-from-Clojure states > defprotocol and deftype, extend-type, extend-protocol work as in Clojure

bronsa12:07:52

clojure's behaviour is intended

bronsa12:07:03

records/types don't live in namespaces

bronsa12:07:10

they're interoppy

bronsa12:07:19

(in clojure)

moxaj12:07:08

this is troublesome - at any rate, I believe the wiki I linked should be updated to document this difference

dnolen13:07:32

@moxaj I’m also not convinced 2109 has anything do with this

dnolen13:07:07

since all that does is ensure in syntax-quoted dotted symbols w/o namespaces don’t change

dnolen13:07:14

which in your minimal repro they are changing

dnolen13:07:43

so file an issue, and unless you’ve git bisected let’s not assume what the cause might be

moxaj13:07:50

seemed very similar on the surface, I didn't investigate further 😕

moxaj13:07:10

> which in your minimal repro they are changing

moxaj13:07:00

they used to, that's why I believed they worked, but they no longer change

moxaj13:07:30

hm, you are right, the version prior to the one with the fix still throws

dnolen13:07:46

I just don’t see how 2109 could be the problem 🙂

moxaj13:07:00

and since cljs-time has fixed it since, I'll leave it at that

dnolen13:07:18

oh so cljs-time fixed this problem?

moxaj13:07:29

I used an old version, transitively

dnolen13:07:52

ok yeah then I’m not concerned - unless people start reporting this one more often

dnolen14:07:37

finally fixed some styling stuff that was driving me crazy on the Quick Start https://clojurescript.org/guides/quick-start

dnolen14:07:10

if there any other docs with weird styling issues, please file issues on GitHub

rauh16:07:15

So one thing I just had to dig in a litle: DCE. GCC doesn't really like it if IIFE's are run and then properties to it attached. For the two equivalent functions one get's DCE'd the other one doesn't:

(defn gen-fn [bf]
  (let [f (fn [] (bf 1))]
    (js* "~{}.x = 1;" f)
    f))
(defn dce-works [spec]
  (let [bf #(-> %1 spec)]
    (gen-fn bf)))

(defn dce-no-worky [spec]
  (let [bf #(-> %1 spec)
        ;; IIFE here with bf as param:
        f (fn [] (bf 1))]
    (js* "~{}.y = 1;" f)
    f))

rauh16:07:16

Note the comment with "IIFE here". This is the very pattern that functions use when returning multi arity variadic functions. So they'll all not get DCE'd.

rauh16:07:31

Also just wanted to share this in case it's of interest to people 🙂

rauh16:07:39

FWIW, I've been sitting on a patch locally to optimize the code created for multi-arity-variadic function expressions. Though I first want to get rid of f.applyTo implementations.

dnolen16:07:53

@rauh the DCE and CMCM (cross module code motion) issues around IIFEs is why we have special top level defn macros

dnolen17:07:02

@anmonteiro did you intend to not handle resolve in the 2183 patch?

dnolen17:07:13

also if you want to lift a helper that’s cool

dnolen17:07:52

ah sorry k missed that

anmonteiro17:07:58

only after wrapping up CLJS-2182 I realized the others missed it too 🙂

dnolen17:07:08

I’ll just apply these, helper is easy to do later

anmonteiro18:07:29

my bad. should have added a test for the happy path

anmonteiro18:07:33

working on one now

dnolen18:07:10

@anmonteiro maybe more pressing - Caused by: clojure.lang.ExceptionInfo: No such namespace: left-pad, could not locate left_pad.cljs, left_pad.cljc, or Closure namespace "left-pad" in file src/test/cljs_build/npm_deps_test/core.cljs

dnolen18:07:19

this happens on master with lein test

dnolen18:07:34

seems a bit odd since I could have sworn I ran lein test after applying the patch

anmonteiro18:07:43

hrm, I tested that repeatedly yesterday. so weird

dnolen18:07:13

I think issue is from an earlier step

dnolen18:07:18

Error: Cannot find module 'npmlog' at Function.Module._resolveFilename (module.js:469:15)

anmonteiro18:07:43

that’s is even stranger 🙂

dnolen18:07:21

I think the other error is just a downstream thing

anmonteiro18:07:30

a common problem is not having a package.json at the root of the project

anmonteiro18:07:37

then NPM doesn’t know where to install stuff

anmonteiro18:07:59

looks like that might be the issue

anmonteiro18:07:09

lein test passes locally after adding a package.json

dnolen18:07:15

trying that here

dnolen18:07:32

I added package.json with just {}

dnolen18:07:35

same error

dnolen18:07:11

my Node.js is messed up

dnolen18:07:29

lemme fix that first

anmonteiro18:07:28

no errors here after adding a package.json with just {}

dnolen18:07:34

trying that by fixing the test to do that

dnolen18:07:43

(my Node.js install was messed up)

dnolen18:07:42

and fixed in master

dnolen18:07:50

thanks for writing these tests 🙂

anmonteiro18:07:22

^ resolve macro tests

dnolen18:07:07

@anmonteiro I thought we had a resolve test, that’s how I ran into this problem

dnolen18:07:12

line 1380 in core_test.cljs

dnolen18:07:19

sorry for not mentioning that earlier

anmonteiro18:07:25

didn’t see that

anmonteiro18:07:30

closing the issue

anmonteiro18:07:34

I thought I had ran the tests

dnolen18:07:06

@anmonteiro CLJS-2181 looks fine - it’s non-invasive like I hoped, any concerns really?

anmonteiro18:07:29

@dnolen nope. Just dwelling whether we should create an ijs or just a plain old IPersistentMap

anmonteiro18:07:41

that would involve creating more defmethods

dnolen18:07:48

nah this is fine for now

dnolen18:07:07

my key concern was keeping the protocol stuff out of cljs.module-graph

anmonteiro18:07:16

yeah I took care that didn’t happen

dnolen18:07:17

like I said in the future we should normalize everything up front

dnolen18:07:27

then we know what we’re looking at in the compiler

dnolen18:07:42

instead of hunting around protocols to figure out what is happening

anmonteiro18:07:01

looking forward to that day 🙂

dnolen18:07:22

it will probably be soon 🙂

dnolen18:07:30

I was close to my last straw getting this module stuff working

anmonteiro18:07:02

btw I held off on writing advanced compilation for npm-deps because of inputs stuff. I think when you output a single file we use io/resource and that wasn’t in the classpath

dnolen18:07:36

you mean advanced compilation test case right?

anmonteiro18:07:12

anyway, that might now be too hard to tackle, probably just move test cases around to dirs that are in the classpath

anmonteiro18:07:45

I might write some more test cases for the build API now that there are some in place that I can build upon

dnolen18:07:47

@anmonteiro 2181 needs a rebase it seem

dnolen18:07:52

3-way merge didn’t work

anmonteiro18:07:43

@dnolen attached rebased patch

anmonteiro18:07:58

problem was npm-deps-test stuff

dnolen18:07:28

I’m starting to wonder what else we need to do for a release ??? 🙂

mfikes18:07:10

Write blog posts

anmonteiro18:07:24

a release is definitely in order!

anmonteiro18:07:44

I have some time in the next hour or so to write up something about string-requires

dnolen19:07:08

@anmonteiro I think your :npm-deps post should include @juhoteperi’s symbol based :process thing

mfikes19:07:23

I’ve also got a half-revised update to the aget post. I’ll finish that draft off so that it is available for review.

anmonteiro19:07:30

ah hrm. I haven’t looked at that

anmonteiro19:07:33

let me look at the commit

dnolen19:07:53

@anmonteiro it’s pretty straightforward and better 🙂

anmonteiro19:07:18

it allows to calls a function in a namespace

dnolen19:07:25

yeah way more sensible

dnolen19:07:39

props to @thheller for the suggestion

juhoteperi19:07:28

Updating the preprocess guide PR is on my TODO

dnolen19:07:06

thinking some more about this it might be nice to do the feature posts and release (right) before EuroClojure?

dnolen19:07:26

gives us time to incorporate smaller issues as they arise, and have some really cool posts leading up to the release.

dnolen19:07:20

(open to other ideas)

dnolen19:07:34

generally it’s nice to have a month or so between releases

juhoteperi19:07:05

Should the preprocess change have it's own blog post?

dnolen19:07:53

@juhoteperi I think that’s OK too

dnolen19:07:03

so that be would 4 posts

dnolen19:07:36

(In no real order) 1) aget/aset 2) :modules 3) :npm-deps string require 4) loadable transforms

dominicm19:07:24

What is aget/aset about? Anything raw I can read?

mfikes19:07:31

@dominicm I’ll have a revision to the draft available soon. The first (fairly minimal) draft is in this PR https://github.com/clojure/clojurescript-site/pull/101

dominicm19:07:28

Oh that's awesome. I'm so glad that's a thing.

dominicm19:07:43

(Mostly because I'm a pedant)

dnolen19:07:04

@dominicm heh in this case it’s a real code smell that appears quite frequently in the wild - so there’s a good opportunity to guide the community in the right direction with this particular post

dominicm20:07:22

dnolen: I will admit to zealously just repeating "but dnolen said don't use aget like that. so don't"

juhoteperi19:07:04

This is now up-to-date with :preprocess change: https://github.com/clojure/clojurescript-site/pull/54

dnolen19:07:49

@juhoteperi nice I think we’ll still want a post too

juhoteperi19:07:53

Just went through the examples with 723 to check they work

anmonteiro19:07:03

@juhoteperi I think we should just write the JS modules post together

juhoteperi19:07:59

Perhaps, lets see when we have something written. I'll go for a walk now but I can write a draft later tonight.

anmonteiro19:07:24

sounds good. I’ll put a rough draft together right now

martinklepsch19:07:56

I feel like spreading posts over a week or two would be most affective in terms of getting attention, just a thought 🙂

dnolen19:07:20

@martinklepsch agree I think doing the posts over two weeks leading up to EuroClojure will be great way to drum up a lot of attention

dnolen19:07:25

this is some really cool stuff 🙂

anmonteiro19:07:02

is EuroClojure in 2 weeks?

dnolen19:07:16

1 day less

dnolen19:07:59

and the second of the conf is the 6th birthday.

martinklepsch20:07:00

EuroClojure is gonna be huge fun. @anmonteiro are you coming?

anmonteiro20:07:12

I’m really bummed that I’m not coming this year 😞

anmonteiro20:07:43

My talk was not selected and I couldn’t justify the trip otherwise 🙂

anmonteiro20:07:56

I’m speaking at ClojuTRE though!

anmonteiro20:07:12

I know you’re a regular there

martinklepsch20:07:24

ah, and you couldn’t plan for an extended stay in Europe? 😛

mfikes20:07:48

https://github.com/clojure/clojurescript-site/pull/101 has been updated with the guidance @dnolen left in the PR. My feeling now is that it might have too much content, so feel free to suggest chopping things out if you end up feeling that way.

mfikes20:07:49

@dominicm Since you seemed interested ^

dnolen20:07:12

@mfikes whoa that already looks awesome 🙂

mfikes20:07:26

Tried to take your theme and make it real 🙂

dnolen20:07:55

my only though is that I think separating these two warnings is a bit strange from the end user perspective

dnolen20:07:03

(makes sense internally of course)

dnolen20:07:19

I’m wondering if we should have a synthetic warning keyword that expands to setting both of these to true?

mfikes20:07:10

Yeah, why did I do that? (Two separate.) I probably didn’t think about it, when I ran into the need to name it :invalid-a[gs]et. But you are absolutely right. It makes no sense to have two.

dnolen20:07:26

another reason to do blog posts 😛

dnolen20:07:32

make sure we’re releasing the thing we want

mfikes20:07:54

Yes. It provides a chance to step away from the implementation and check for sanity.

dnolen20:07:17

but otherwise this looks great

mfikes20:07:05

:invalid-array-access (access being get or set)

mfikes20:07:12

I’ll put together a patch. It might be feasible to drive the warning that gets emitted via a single warning key. But, yeah the public API should be just one key, no matter what we do internally.

dnolen20:07:58

@mfikes your post has got me really excited about the News section of the ClojureScript site 🙂

mfikes20:07:09

Yeah. It seems like a great outlet for things ranging from highlight new cool features to stuff on the philosophical end of the spectrum.

dnolen20:07:18

@anmonteiro haha I’m sure it will be

anmonteiro20:07:27

@juhoteperi if you want to collaborate on this ^ I can give you access to my fork

dnolen20:07:23

@anmonteiro I’m thinking we probably want this to be separate posts now 🙂

dnolen20:07:32

I left a bunch of feedback and your post could be longer

anmonteiro20:07:56

@dnolen just saw your reply. that’s great feedback, I’ll certainly incorporate it

dpsutton20:07:21

just from an outsiders perspective its really cool watching the enthusiasm in here and how you all work together

dnolen20:07:37

@juhoteperi’s post can link to yours, should quickly summarize the JS modules points - and highlight how this takes the :npm-deps work even further since we can leverage all the JS transform stuff

dnolen20:07:01

I think super important to make it clear that all this stuff works together

dnolen20:07:11

(not assume much prior knowledge in the reader)

anmonteiro20:07:37

definitely. I’ll have another draft ready later today or tomorrow

anmonteiro23:07:40

noticed we didn’t have a test for :preloads, went ahead and added one https://dev.clojure.org/jira/browse/CLJS-2189