Fork me on GitHub
#cljs-dev
<
2017-08-09
>
dnolen13:08:37

@kommen probably just an oversight, though I am surprised it wasn’t caught by @anmonteiro work on this

dnolen13:08:55

we should munge reserved keywords consistently as long you haven’t set :language-out to >= :es5

kommen13:08:10

yeah, I was surprised that just requiring the compiler apparently changes how stuff is munged

dnolen13:08:31

@kommen I don’t follow, bootstrapped is a different thing

dnolen13:08:38

are we only talking about bootstrapped here?

dnolen13:08:49

we should be clear and keep bootstrap and non-bootstrap issues separate

dnolen13:08:04

the later is more important, and the former usually gets sorted out later

kommen13:08:44

no, not boostrapped (if I understand correctly)

kommen13:08:18

so it happens for me when compiling the sample with uberjar built from the current master

dnolen13:08:40

I don’t know what you mean by “just requiring the compiler”

dnolen13:08:03

you don’t require the compiler unless you’re talking about bootstrapped

dnolen13:08:12

(I might just be misunderstanding you)

kommen13:08:40

ok, let me clarify

kommen14:08:02

@dnolen oh you were right: it’s actually an issue with :language-out and unrelated to any requires. I just didn’t catch this as changing the :language-out option doesn’t recompile the file without killing the out directory first

dnolen14:08:21

@kommen ok good to know

dnolen14:08:48

I think there’s still an issue here, but can ask António about that later

kommen14:08:41

I’ll comment on the issue

kommen14:08:20

thanks, just wanted to ask about that 🙂

anmonteiro15:08:26

So my work on default was after 1.9.854

anmonteiro15:08:31

This seems jnrelated

anmonteiro15:08:04

Unless @kommen built from master?

kommen16:08:59

@anmonteiro yep, I built from master

anmonteiro16:08:38

I see, that wasn’t clear

anmonteiro16:08:45

probably a bug in my patch then

anmonteiro16:08:54

I’ll look into it before Friday

anmonteiro16:08:52

@dnolen @kommen so the thing about CLJS-2312

anmonteiro16:08:01

that’s not valid ES5

anmonteiro16:08:29

I renamed every param named default

anmonteiro16:08:25

so the problem here is that we may only want to allow default as a property access

kommen16:08:05

@anmonteiro did you miss this occurrence of default i linked to in the issue?

dnolen17:08:57

@anmonteiro maybe this wasn’t in your patch, but I do think related - we should consistently munge default if the language is not >= :es5.

dnolen17:08:09

basically always default$ it should never be sometimes

anmonteiro17:08:33

I think we’re doing that

anmonteiro17:08:46

or, I’m pretty sure we’re always munging default

anmonteiro17:08:56

but there’s definitely a bug here

anmonteiro17:08:08

even when language >= :es5 we can’t just not munge default

anmonteiro17:08:21

because it’s only allowed as a property access

anmonteiro17:08:34

function foo(default) {console.log(default)} is not valid ES5

anmonteiro17:08:42

but someObject.default() is

anmonteiro17:08:53

@dnolen does this make sense?

anmonteiro17:08:26

cool, working on a fix

anmonteiro17:08:49

and a regression test, ofc

dnolen17:08:50

@anmonteiro what about deftype fields?

dnolen17:08:16

I think if no >= :es5 we should warn

anmonteiro17:08:19

I may not have enough context about those

dnolen17:08:26

it’s an old bug

dnolen17:08:39

my decision was that we should warn if people create JS deftype with reserved field names

dnolen17:08:18

there’s actually like 3 tickets about this

dnolen17:08:25

but yes that’s one of them

dnolen17:08:41

the only tricky thing I think is that we need to be careful with deftype constructor fn

dnolen17:08:49

to munge the param, but not the field

anmonteiro17:08:11

“default constructor”?

dnolen17:08:23

sorry typo

dnolen17:08:34

in anycase, this is a second step thing

anmonteiro17:08:48

I think we should fix the immediate bug that @kommen reported

anmonteiro17:08:59

because it’s pretty bad and shouldn’t be included in the release

anmonteiro17:08:15

and worry about the deftype things later

dnolen17:08:29

sounds good

anmonteiro17:08:30

since it’s not a regression we introduced between 1.9.854 and current master

anmonteiro17:08:33

actually a good regression test is just reverting all those default renames

anmonteiro17:08:11

I think it’s safe to assume that if var-name has a namespace, we’re going to emit dotted property access

anmonteiro17:08:15

so we don’t need to munge default in those cases

mhuebert17:08:12

is cljs supposed to install :npm-deps that are specified?

mhuebert17:08:52

didn’t see that, thanks.

mfikes17:08:49

What confuses me is the docs for :npm-deps indicates Used to install NPM dependencies on behalf of the user. Perhaps it should say something used to declare? Hmm.

mfikes17:08:34

Actually that’s how the description starts. Declare NPM dependencies. …

anmonteiro17:08:50

it used to install the deps

anmonteiro17:08:53

but we changed the default behavior

mfikes17:08:03

Yep. Maybe that sentence could be removed, or revised…

mhuebert17:08:46

is there documentation somewhere for what happens with conflicting npm versions?

anmonteiro18:08:05

we have simple conflict detection

anmonteiro18:08:22

@mhuebert here’s the thing. :npm-deps is supposed to be used for very simple use cases

mfikes18:08:24

I’ll submit a PR that removes the sentence. If accepted it would read Declare NPM dependencies. A map of NPM package names to the desired versions. See also :install-deps.

mhuebert18:08:29

I’m wondering what to do about a bunch of interrelated libs that all use React

anmonteiro18:08:35

if you have a more complex setup, just install the dependencies yourself

anmonteiro18:08:45

and require them in your ClojureScript code

mhuebert18:08:54

so you don’t have to list them in :npm-deps to use them

anmonteiro18:08:01

The CLJS compiler is smart enough to know which things are node_modules

mhuebert18:08:04

ah that would make life easier. I thought it was required

mhuebert18:08:32

so putting things in node_modules is the equivalent of adding something to the classpath

anmonteiro18:08:42

I guess you could put it that way

mhuebert18:08:09

👍:skin-tone-2:

anmonteiro18:08:48

maybe that can help you set things up

anmonteiro18:08:22

note that installing @cljs-oss/module-deps and konan is required if you’re targeting the browser

dnolen18:08:17

@mfikes, thanks merged

dnolen19:08:17

@anmonteiro huh I see they just tagged Closure Compiler, is that pre your patch?

anmonteiro19:08:51

@dnolen unfortunately yes. Well. It has the simple usage

anmonteiro19:08:02

Not yesterday's patch though

dnolen19:08:18

ok, was just curious

dnolen19:08:45

@anmonteiro I guess they do have SNAPSHOT releases?

anmonteiro19:08:14

Yeah but that's risky

anmonteiro19:08:26

Their snapshot is always 1.0-SNAPSHOT

anmonteiro19:08:37

and updated on every CI run

anmonteiro19:08:47

So a broken commit and you're done

dnolen19:08:02

k thanks for the clarification

anmonteiro19:08:30

Having our own artifact would let us track master or whatever revision works best for us

dnolen19:08:25

yes, just wanted to make sure there wasn’t another way