Fork me on GitHub
#cljs-dev
<
2017-08-03
>
anmonteiro02:08:19

so good news, bad news

anmonteiro02:08:51

good news is I have a patch that adds an option to Closure making the browser field (or any field, really) work

anmonteiro02:08:18

bad news is that calling .toSource on converted warning NPM module results in a Closure error

anmonteiro02:08:52

^ wait this is not true

anmonteiro02:08:51

oh it’s the UMD thing

anmonteiro02:08:08

@juhoteperi was this the error you were getting in the UMD thing?

Caused by: java.lang.IllegalStateException
        at com.google.common.base.Preconditions.checkState(Preconditions.java:429)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:657)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:638)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:106)
        at com.google.javascript.jscomp.CodeGenerator.addFunction(CodeGenerator.java:1363)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:384)
        at com.google.javascript.jscomp.CodeGenerator.addExpr(CodeGenerator.java:1556)
        at com.google.javascript.jscomp.CodeGenerator.addStringKey(CodeGenerator.java:1631)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:960)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:106)
        at com.google.javascript.jscomp.CodeGenerator.add(CodeGenerator.java:987)

dnolen11:08:22

@anmonteiro nice one on the Closure PR

juhoteperi12:08:52

I'm testing if the UMD wrapper changes are enough, because I noticed React and others use similar pattern where module.exports is assigned result of a function call, and somehow that already works

dnolen12:08:58

@juhoteperi there’s nothing wrong with that pattern far as I know

dnolen12:08:02

it’s not really dynamic

juhoteperi12:08:39

Well, Closure fails during .toSource call for some file currently, with that change, so something isn't working

juhoteperi12:08:14

Trying to debug further which file causes the problem

juhoteperi12:08:05

Oh I forgot React doesn't really have any UMD wrapper when using code from npm package

juhoteperi12:08:57

I don't think the module processing knows about constants

thheller14:08:11

@juhoteperi https://unpkg.com/[email protected]/index.js it does work but :none optimized code will also load the production code

thheller14:08:25

since we can’t do conditional require

thheller14:08:36

for :advanced the development stuff gets optimized away

thheller14:08:40

would be neatto skip the index.js but not sure how without manual tricks

juhoteperi14:08:34

I think Closure just has to be made aware of that pattern

thheller14:08:11

I hope thats not really a pattern and react removes it again

thheller14:08:30

never saw it anywhere else at least

juhoteperi14:08:48

Based on that looks like they are planning on using this

thheller14:08:50

wish there was an agreed standard in package.json for this

thheller14:08:06

something like main, browser etc

thheller14:08:26

but I guess we have to work around it a bit

thheller14:08:55

only real issue is that :none loads the rewritten react/umd/react.production.min.js but does not use it

thheller14:08:11

and that :advanced has to optimize away the react/umd/react.development.js

anmonteiro15:08:09

just rebased cljs-2291

anmonteiro15:08:10

@bronsa just saw your comment in cljs-2261

anmonteiro15:08:25

but I don’t understand it given the commit

anmonteiro15:08:39

it seems that you’re still passing the symbol without the dot to resolve-symbol

bronsa15:08:53

the important part is that the 3 deletions at the bottom

bronsa16:08:07

we're now not special casing resolution of foo. in syntax-case, and just delegating to resolve-symbol

anmonteiro16:08:37

do you think it matters for ClojureScript though?

anmonteiro16:08:48

given the macro problem we discussed the other day

bronsa16:08:03

well, it gives you the ability to support it in self-hosted if you want

bronsa16:08:14

and should help eliminating the warning

anmonteiro16:08:23

1.0.4 already worked in self-hosted

anmonteiro16:08:47

I can see how this is more correct than the previous version though

bronsa16:08:56

yeah but caused a regression in CLJ, this allows the 1.0.4 fix in cljs and fixes the behaviour for clojure

anmonteiro16:08:11

awesome, thanks for clarifying

bronsa16:08:18

re: having this work in cljs JVM

bronsa16:08:26

i have a prototype of require that uses tools.reader

bronsa16:08:46

unfortunately as I was writing it, I realized that transitive requires would still be loaded through c.c/require

bronsa16:08:50

so I'm not sure it's worth it

anmonteiro16:08:11

that makes sense

bronsa16:08:42

in tools.emitter.jvm I worked arount this by with-redefing c.c/load

bronsa16:08:50

I wouldn't really do that

dnolen21:08:03

@anmonteiro 2291 doesn’t apply cleanly for me, do I need to do something to make sure this works?

anmonteiro21:08:12

let me check again

dnolen21:08:16

@mfikes what was that command for applying windows file patches?

mfikes21:08:39

Hmm... something about newlines...

anmonteiro21:08:18

doesn’t apply for me

anmonteiro21:08:21

rebasing again

dnolen21:08:18

@anmonteiro ^ fixes it for me - applies

anmonteiro21:08:48

yeah same here

dnolen21:08:48

now to remember that 🙂

anmonteiro21:08:02

I’ll add a PR to the CLJS site

anmonteiro21:08:34

hopefully we won’t forget where to look for it next time

anmonteiro21:08:39

I’m surprised Slack saved the messages above form July 9th

dnolen21:08:58

is 2261 ready?

anmonteiro21:08:31

good to go, TR 1.0.5 signed off by Nicola

anmonteiro21:08:11

@mfikes if you register the ClojureScript project in your Appveyor account

anmonteiro21:08:16

you’ll get CI for Windows too 🙂

anmonteiro21:08:34

2291 just went in

dnolen21:08:51

@juhoteperi would like some more context on 2294

dnolen21:08:55

left a comment

dnolen21:08:46

gotta run for now

anmonteiro22:08:04

FWIW Windows CI is now running when I push to my fork at https://github.com/anmonteiro/clojurescript/commits/master

anmonteiro22:08:33

I’ll be working on more patches to make the remaining test failures pass

anmonteiro22:08:09

goal is to have the Windows CI happy / at parity with Travis

anmonteiro22:08:54

here’s a list off the top of my head of what’s missing in the Windows CI: - self-host tests - self-parity tests - set up more JS engines (only spidermonkey is running atm) - set up a way to check runtime test output in a similar vein to what @mfikes did with Travis (`grep` for "0 failures, 0 errors")

anmonteiro22:08:14

maybe we can turn this into one or several JIRA tickets ^

mfikes23:08:11

By adding a little meta (`^:numeric ^{:js-op "max"}`) to the js* form inside the max macro, we can recover a little of the intent of ::ana/numeric:

cljs.user=> (max 1 "a")
WARNING: max, all arguments must be numbers, got [number string number string] instead. at line 1144 <cljs repl>
"a"
But, it is a bit of a hack, IMHO.