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

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

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

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

in what respect

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

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

if it worked, you just got lucky

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

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

clojure's behaviour is intended

records/types don't live in namespaces

they're interoppy

(in clojure)

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

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

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

which in your minimal repro they are changing

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

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

> which in your minimal repro they are changing

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

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

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

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

oh so cljs-time fixed this problem?

yes

I used an old version, transitively

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

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

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

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

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.

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

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.

(Preview: https://gist.github.com/rauhs/ee1609ecb7cbe1bddd57f9469ddd0f48 )

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

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

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

@dnolen I tackled resolve in https://dev.clojure.org/jira/browse/CLJS-2182

ah sorry k missed that

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

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

👍

@dnolen hrm https://github.com/clojure/clojurescript/commit/6a6cdf1292414624271c189d4477e5f0eba77016

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

working on one now

@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

this happens on master with lein test

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

hrm, I tested that repeatedly yesterday. so weird

I think issue is from an earlier step

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

that’s is even stranger 🙂

I think the other error is just a downstream thing

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

then NPM doesn’t know where to install stuff

looks like that might be the issue

lein test passes locally after adding a package.json

trying that here

I added package.json with just {}

same error

oh rewind

my Node.js is messed up

lemme fix that first

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

trying that by fixing the test to do that

(my Node.js install was messed up)

confirmed

and fixed in master

thanks!

thanks for writing these tests 🙂

@dnolen https://dev.clojure.org/jira/browse/CLJS-2187

^ resolve macro tests

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

line 1380 in core_test.cljs

uh

sorry for not mentioning that earlier

didn’t see that

closing the issue

I thought I had ran the tests

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

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

that would involve creating more defmethods

nah this is fine for now

👍

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

yeah I took care that didn’t happen

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

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

instead of hunting around protocols to figure out what is happening

yep

looking forward to that day 🙂

it will probably be soon 🙂

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

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

you mean advanced compilation test case right?

yeah

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

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

@anmonteiro 2181 needs a rebase it seem

3-way merge didn’t work

looking

@dnolen attached rebased patch

problem was npm-deps-test stuff

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

Write blog posts

yeah

a release is definitely in order!

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

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

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.

ah hrm. I haven’t looked at that

@mfikes cool

let me look at the commit

@anmonteiro it’s pretty straightforward and better 🙂

ooooh

it allows to calls a function in a namespace

yeah way more sensible

props to @thheller for the suggestion

Updating the preprocess guide PR is on my TODO

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

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

(open to other ideas)

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

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

@juhoteperi I think that’s OK too

so that be would 4 posts

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

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

@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

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

(Mostly because I'm a pedant)

@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

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

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

Just went through the examples with 723 to check they work

Yeah

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

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

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

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

^ agreed

@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

this is some really cool stuff 🙂

is EuroClojure in 2 weeks?

1 day less

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

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

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

ah, bummer

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

I’m speaking at ClojuTRE though!

I know you’re a regular there

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

aw man!

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.

@dominicm Since you seemed interested ^

@mfikes whoa that already looks awesome 🙂

Tried to take your theme and make it real 🙂

@mfikes this rocks

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

(makes sense internally of course)

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

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.

another reason to do blog posts 😛

make sure we’re releasing the thing we want

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

but otherwise this looks great

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

yeah

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.

thanks!

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

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

@dnolen hope this is equally exciting: https://github.com/clojure/clojurescript-site/pull/104

@anmonteiro haha I’m sure it will be

https://clojurescript.org/news/2017-07-07-sneak-preview

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

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

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

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

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

@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

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

(not assume much prior knowledge in the reader)

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

cool!

gotta run

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