Fork me on GitHub
#cljs-dev
<
2015-08-03
>
martinklepsch09:08:42

@dnolen: the macro question in #C03S1L9DN originated from the stuff above. with @bensu’s help I figured out that using a ` wasn’t the right thing here:

(defmacro define
  [sym default]
  (let [defname (-> (str *ns* "." (name (symbol sym)))
                    (clojure.string/replace "-" "_"))
        goog-define (str "/** @define {boolean} */"
                         "goog.define('" defname "', " default")")]
    (list 'do
          (list 'declare (symbol sym))
          (list 'js* goog-define))))

dnolen10:08:48

@martinklepsch: re, CLJS-1389 before spending any more time on a particular solution I would first explore why just emitting CLOSURE_DEFINES under :none is not satisfactory

dnolen10:08:29

@martinklepsch: adding any new api should always be considered as a last resort

martinklepsch10:08:20

@dnolen: the problem is that overriding defines with CLOSURE_DEFINES only works for defines done with goog.define. So far so good. Now they still need the @define annotation. Plus when using goog.define you don’t actually declare a var causing undeclared var warnings down the line.

dnolen10:08:42

@martinklepsch: how did you come to this conclusion

dnolen10:08:53

the ticket has no links, nothing backing up any claims

martinklepsch10:08:02

ticket has a link

dnolen10:08:07

based on all threads about this issue previously what you are saying is simply not true

dnolen10:08:48

that link doesn’t prove anything

dnolen10:08:58

if you read the source it’s obvious you don’t need goog.define

martinklepsch10:08:00

I see that there’s a part missing. let me check.

dnolen10:08:17

all you need to supply is CLOSURE_DEFINES

dnolen10:08:24

goog.define is just sugar.

dnolen10:08:39

@martinklepsch: I’ve commented on the ticket what I think should be done.

dnolen10:08:47

it would require users to do nothing new

martinklepsch10:08:28

@dnolen: I think I did exactly the change you suggest and ran into problems but let me check again first

dnolen10:08:52

@martinklepsch: again all threads I have ever read say what I suggested work

dnolen10:08:04

if you can prove that it can’t the ticket needs to include documentation demonstrating this

dnolen10:08:02

@martinklepsch: looks like someone forgot to elevate your privileges in JIRA simple_smile

dnolen10:08:18

you can do pretty much anything to tickets I can outside of real admin stuff

dnolen10:08:36

I’ve assigned the ticket to you, assign it back to me whenever you like simple_smile

martinklepsch10:08:00

great. thank you

dnolen11:08:57

@martinklepsch: rationale is much better now, in general tickets should proceed with that before proceeding further.

dnolen11:08:32

@martinklepsch: so it seems like we’ll want the define helper macro + support to construct CLOSURE_DEFINES from the compiler options under :none

martinklepsch11:08:51

yeah? The macro was just an experiment really. You think it’s the best option?

dnolen12:08:14

not sure how you can supply the JSDoc bit otherwise

martinklepsch12:08:14

The CLOSURE_defines stuff is trivial really.

dnolen12:08:54

there’s no benefit to this being a function

dnolen12:08:04

goog.define is a special thing like goog.provide or goog.require

martinklepsch12:08:11

yeah, wasn’t thinking about function but rather having something like (def ^:goog-define thing true) or so. That said I have no clue if that would be a good or terrible idea.

dnolen12:08:55

@martinklepsch: that’s a more invasive change not less

martinklepsch12:08:28

@dnolen: ok. will try to smoothen that macro then.

martinklepsch12:08:30

@dnolen: is there some fn that converts a cljs my.thing/sym? to my.thing.sym_?

dnolen12:08:03

cljs.compiler/munge

dnolen12:08:10

for this safe to use since it’s an internal thing

martinklepsch12:08:44

@dnolen: The right place to add this would be core.cljc is that correct? What’s the “flow” of making changes in there? evaluating the ns form in a repl throws a bunch of stuff at me.

dnolen12:08:21

@martinklepsch: never had any issues loading that file in a REPL

martinklepsch12:08:24

ok, must be something with inf-clojure then

martinklepsch13:08:37

@dnolen: can you point me to a macro that is similar to the define one? Don’t quite understand what to scope in :cljs conditionals and what not

dnolen13:08:32

@martinklepsch: don’t worry about :cljs for now

dnolen13:08:49

making anything new work in bootstrapped is super low priority

martinklepsch13:08:54

ah, k. This is what I have right now: https://gist.github.com/martinklepsch/2e4a20e98ead0ed6953d but it gives be Unable to resolve symbol: js* in this context, — mostly took inspiration from the other macros and they seem to do it the same way?

dnolen13:08:16

@martinklepsch: that macro seems unnecessarily complicated to me

martinklepsch13:08:28

tbh, my macro skills are non-existent — very happy to be educated simple_smile

dnolen13:08:28

@martinklepsch: the less you do with js* the better

dnolen13:08:43

(goog/require …) preferred

dnolen13:08:01

(goog/define …) sorry

dnolen13:08:16

the only thing you want js* for is the JSDoc bit

martinklepsch13:08:15

yeah makes sense.

dnolen13:08:53

@martinklepsch: as far js* not working, see the arithmetic macros like +

martinklepsch14:08:56

@dnolen: my problems with js* not working weren’t caused by the macro stuff but rather by things like calling str instead of core/str. simple_smile

martinklepsch14:08:33

this also causes exceptions with js* being unresovable which confused me simple_smile

dnolen14:08:47

ah right, yeah cljs/core.cljc is a bit tricky now because of the circularity

martinklepsch14:08:15

I have something that works now: https://gist.github.com/martinklepsch/2e4a20e98ead0ed6953d — feedback appreciated

dnolen14:08:32

so that + the necessary changes to cljs.closure is patch-worthy

martinklepsch14:08:05

cool. will upload something to JIRA in a bit then

martinklepsch14:08:25

@dnolen: is main injected during :whitespace?

dnolen14:08:42

no only for :none

martinklepsch14:08:01

because in theory CLOSURE_DEFINES would need to be present for whitespace as well

dnolen14:08:20

making this work with :whitespace is a nice to have not essential

martinklepsch14:08:37

at least as far as I understood — :whitespace is still uncompiled in Closure lingo right?

dnolen14:08:40

:none, :simple, :advanced are the modes we really care about

dnolen14:08:51

@martinklepsch: I believe so but would need to verify

dnolen14:08:55

I never use :whitespace

martinklepsch14:08:05

yeah agree, just making sure things are clear

mikepence14:08:21

I am hoping to contribute some effort to cljs development. what is the best way to get oriented as a newb?

dnolen14:08:55

this low hanging fruit query, not much there at the moment

dnolen14:08:33

@mikepence: make sure you have sent in your CA. Then scan through the tickets and JIRA and pick something that tickles your fancy

dnolen14:08:47

if you have questions ask here or on ClojureScript IRC

dnolen14:08:59

I and others that work on the compiler can help

mikepence14:08:39

how do I get a CA?

mikepence14:08:28

nevermind, found it

martinklepsch15:08:08

@dnolen: can’t get it to work in node. Does the node_bootstrap.js somehow create a new context where the previously set vars aren’t available anymore?

dnolen15:08:34

@martinklepsch: remember Node.js has modules

dnolen15:08:51

you need to be careful how you create CLOSURE_DEFINES so everything can actually read it

dnolen15:08:25

goog.global holds the global context in platform specific way

dnolen15:08:42

so make sure you set a property on that

martinklepsch15:08:14

so var goog.global.CLOSURE_DEFINES vs. just var CLOSURE_DEFINES basically?

dnolen15:08:22

well sans var

martinklepsch15:08:30

ah right yeah.

martinklepsch15:08:32

Dunno much about node tbh

dnolen15:08:45

if you really want something to global write to goog.global

dnolen15:08:55

in browser js/window of course, but in Node won’t work

dnolen15:08:05

goog.global gets you out of caring

martinklepsch15:08:52

@dnolen: thanks, that helped

martinklepsch16:08:35

@dnolen: I added a patch. Can add a second one with the sample thing if wanted.

martinklepsch17:08:17

@dnolen: has been fun! thanks for all your guidance simple_smile

dnolen18:08:47

@martinklepsch: patch looks OK, just needs to be rebased to master.

martinklepsch18:08:44

@dnolen: done. not sure if how I can remove the old patch

dnolen18:08:58

@martinklepsch: I tweaked the docstring, we actually munge :closure-defines for the user in cljs.closure/add-implicit-options

dnolen19:08:29

if some tool is not passing build options through cljs.closure/add-implicit-options that’s a bug

martinklepsch19:08:56

ah, great. I somehow assumed there is no processing being done there.

martinklepsch19:08:23

should have checked the claim that it doesn’t work simple_smile

dnolen19:08:11

@martinklepsch: thanks for the patch, nice enhancement

martinklepsch19:08:59

Happy to get it in. Parameterized builds without custom macros and env vars across all optimization modes is really handy simple_smile

dnolen19:08:07

it sure is, low hanging fruit like this is definitely some of the most impactful contribution

dnolen21:08:37

@martinklepsch: going to rename define to goog-define to make it more clear. cljs.core/define is just a bit too weird semantically

martinklepsch21:08:26

yeah, I was considering suggesting that but you seemed fine with it so 👍

martinklepsch21:08:47

define indeeed seems weird given that def is basically saying the same

martinklepsch21:08:12

anyways, go for it. simple_smile