Fork me on GitHub
#cljs-dev
<
2017-08-16
>
dnolen05:08:22

@anmonteiro I think that’s a refactor for later, gonna close CLJS-2309 for now

anmonteiro05:08:48

it’s an optimization

symfrog08:08:35

CLJS-2309 is now resolved on my side too when using master. @anmonteiro fwiw, I was able to reproduce the issue using the standalone 1.9.854 cljs.jar , it can be triggered by specifying a long file path to a packaged foreign lib on the classpath, I have added a script to the repo https://github.com/symfrog/cljs-modules-react-order/blob/master/standalone.sh that will produce ./release/fail_moduleb.js and ./release/pass_moduleb.js simply by varying the file path to the jars

dnolen13:08:55

reminder I’m planning on cutting a release today - now would be a good time to test

juhoteperi14:08:12

Btw. I guess Tsickle also allows converting whole TS projects to Closure

juhoteperi14:08:19

> We already use tsickle within Google to minify our apps (including those using Angular), but we have less experience using tsickle with the various JavaScript builds that are seen outside of Google.

honzabrecka15:08:53

Just an update: https://github.com/honzabrecka/ts-to-goog is now rewritten to clojure and repository contains externs (~3.5k) generated from DefinitelyTyped. Take a look and let me know.

richiardiandrea15:08:46

it is very useful thanks!

martinklepsch16:08:32

super cool! was chatting about this with a friend some time ago, excited to see it being done 🙂

Neel12:03:42

After executing this command: java -jar gen.jar <npm-package> , it deletes the npm-modules folder and package.jason file and throws npm-packages not found error. Has anyone ran into this problem, any solution?

dnolen15:08:48

@anmonteiro hey I’m removing the konan dep and just putting it inline into module_deps.js

dnolen15:08:24

I’m also reformatting this file to 4 space indent to match our other JS sources, and making it consistently ES6 - I don’t see any problems with this at the moment - but maybe you know something

anmonteiro16:08:06

Consistently es6?

anmonteiro16:08:20

Like using const and things?

dnolen16:08:24

just switching var to let etc.

dnolen16:08:32

so IntelliJ isn’t so annoying when looking at this file

anmonteiro16:08:47

I've refrained from doing that because of older Node.js versions

dnolen16:08:03

but konan is ES6 already

dnolen16:08:11

so how would that work?

juhoteperi16:08:35

It doesn't. I updated from Node 4 to 6/8 some weeks ago.

anmonteiro16:08:40

Hah missed that

dnolen16:08:46

konan uses const, default params, method defs in object literals

dnolen16:08:18

@anmonteiro I’m actually fine with lowering to ES5

anmonteiro16:08:20

Ok so we require Node 6, which is the LTS anyway

dnolen16:08:31

so we can support pre Node 6

dnolen16:08:36

another reason to get off konan

anmonteiro16:08:51

@dnolen OK cool. Do you wanna address this for today's release?

juhoteperi16:08:00

I don't think anyone really uses Node 4 on work machines

juhoteperi16:08:10

Some servers might run that but nothing really works with that anyway

dnolen16:08:13

I’m also fine w/ saying Node 6 min

anmonteiro16:08:16

My thinking is we put that code in our module-deps fork

anmonteiro16:08:23

Not in the cljs codebase

dnolen16:08:08

@anmonteiro we could, but I’m actually more interested in just getting export ... from working

dnolen16:08:13

cleanup can happen later

anmonteiro16:08:20

@dnolen sure, but if it's as easy as you make it sound I can do it before lunch

anmonteiro16:08:27

And still be in the release

dnolen16:08:45

@anmonteiro ok, then first let me get my fix in, and you can provide a refactor patch

anmonteiro16:08:54

Whatever you prefer

dnolen16:08:02

@anmonteiro this was the only change konan needed

anmonteiro16:08:50

Does that handle exporting default declarations?

anmonteiro16:08:03

Dunno if the AST node is the same type

anmonteiro16:08:15

But if it's not it should be included

anmonteiro16:08:10

@dnolen i.e. I think you're handling export {x} from 'y'; but not export default from 'y'

dnolen16:08:13

hrm good point

dnolen16:08:39

Closure Compiler IllegalStateException is not forthcoming

dnolen16:08:49

is there anyway to get more useful information from this?

dnolen16:08:37

@anmonteiro I pushed my work, far as I can, old stuff is still working fine - tests are passing

dnolen16:08:30

for the time being I’d actually like to keep this work in ClojureScript, it’s just simpler to make changes - it also gives a better picture how we might fold this into ClojureScript itself by rewriting this stuff in Clojure

dnolen16:08:42

gonna grab some lunch bbiab

dnolen16:08:16

oh material is still not working, but it’s due to some IlllegalStateException now I’ll have to look at when I get back

anmonteiro16:08:51

@dnolen IllegalStateExceptions are a pain, but I can try and look if you point me to a stacktrace

anmonteiro16:08:12

normally it’s a call to Guava’s checkState that doesn’t provide any other information

bendlas16:08:30

hm, konan's author already resolved the issue: https://github.com/egoist/konan/issues/4

anmonteiro16:08:29

@dnolen one other thing. Do we now require more deps to be installed?

anmonteiro16:08:41

we should list those in @cljs-oss/module-deps

dnolen17:08:32

@bendlas nice, but really no need to depend on konan, it’s a really a trivial lib

dnolen17:08:40

@anmonteiro already did that

anmonteiro17:08:28

@dnolen fwiw the konan author also didn’t handle default exports

anmonteiro17:08:37

I’m not even sure if that is part of the import spec yet

dnolen17:08:53

if people are already doing that should probably handle?

anmonteiro17:08:55

it’s only at stage 1

anmonteiro17:08:59

I don’t think we need to handle

rarous17:08:43

But it is widely used

dnolen17:08:05

@rarous any interesting major libs that do?

dnolen17:08:17

just because people are using them in their apps - less concerned about that

rarous17:08:06

thats true, sorry for derailing 🙂

dnolen17:08:55

@anmonteiro if we can get something else non-trivial like Material to work that would be another big step 🙂

thheller18:08:48

@material/drawer fails with an exception for me

thheller18:08:51

IllegalStateException: 
	com.google.common.base.Preconditions.checkState (Preconditions.java:429)
	com.google.javascript.jscomp.CodeGenerator.add (CodeGenerator.java:986)
	com.google.javascript.jscomp.CodeGenerator.addExpr (CodeGenerator.java:1556)
	com.google.javascript.jscomp.CodeGenerator.add (CodeGenerator.java:274)

thheller18:08:19

it doesn’t seem to like node_modules/@material/drawer/temporary/constants.js with the export that only has the key

thheller18:08:25

I use this to resolve the deps https://gist.github.com/thheller/d5761ee8adbe6f857bf6e05a71efda06 which already handles the export from ... stuff

thheller18:08:43

it works when I change FOCUSABLE_ELEMENTS, to FOCUSABLE_ELEMENTS:FOCUSABLE_ELEMENTS,

thheller18:08:07

don’t know if its my settings or an actual bug though

anmonteiro18:08:21

probably a closure bug?

anmonteiro18:08:32

it also fails for me in ‘./persistent’

anmonteiro18:08:52

oh it also has constants

anmonteiro18:08:34

setting language-in to ecmascript-next doesn’t have any effect

anmonteiro18:08:03

looks related

anmonteiro18:08:12

perhaps low hanging fruit in Closure that one of us here can help solve

dnolen18:08:00

@anmonteiro does that appear in Material?

anmonteiro18:08:01

yes, let me grab the link

anmonteiro18:08:57

this is what Thomas was referring to

anmonteiro18:08:17

that’s what Closure is choking on

dnolen18:08:23

@anmonteiro huh, what is going on there?

dnolen18:08:51

is that valid ES6 object literal syntax now?

dnolen18:08:09

at least we know TC39 is working on important problems

anmonteiro18:08:19

shorthand for var a = 1; {a: a}

anmonteiro18:08:32

you can just specify {a}

anmonteiro18:08:13

I guess it’ll be unsupported in Closure until they add parser support for the new stuff

anmonteiro18:08:29

the issue is fairly new, like 1 month old

anmonteiro18:08:32

maybe it’s being worked on

anmonteiro18:08:52

or maybe a CLJS person needs to work on it for them 😛

dnolen18:08:11

this says shorthand properties work?

dnolen18:08:14

basics seem to work fine

dnolen18:08:42

also seems to work fine

dnolen18:08:55

can’t find about anything constants.js that doesn’t seem to already work

dnolen18:08:40

@anmonteiro how were you able to determine that the issue was constants.js?

anmonteiro18:08:05

@dnolen if I change the line to read FOCUSABLE_ELEMENTS: FOCUSABLE_ELEMENTS it passes

anmonteiro18:08:15

like actually modify node_modules

anmonteiro18:08:26

both in persistent/constants and temporary/constants

dnolen18:08:51

I have a feeling this isn’t a parsing problem

dnolen18:08:56

but an export problem

anmonteiro19:08:23

trying something else

dnolen19:08:04

yeah it has to be, since the illegal state exception is from codegen, after parsing

dnolen19:08:11

I think this is because we’re just rewriting ES6 module syntax

anmonteiro19:08:37

by us you mean Closure?

dnolen19:08:16

well Es6RewriteModules.java

anmonteiro19:08:26

might be that

dnolen19:08:37

well anyways this isn’t going to get solved right now … I’d say the the change I made is for the better and will allow more things to be consumed

dnolen19:08:41

anybody against a release?

dnolen19:08:25

stepping away for a bit, but planning on cutting a release in an hour or so

mfikes19:08:44

Canary passes ✔️

anmonteiro20:08:38

@dnolen agreed it won’t change soon. I can look into it on the Closure side, maybe later

anmonteiro20:08:51

I’m 👍 for a release

dnolen21:08:30

cutting a release now

dnolen22:08:14

ok I’m probably going to lay low for the rest of the week and will be out most of next, technically already on vacation 🙂

anmonteiro23:08:37

@dnolen just reproed the material bug in a Closure compiler test

anmonteiro23:08:44

definitely in Es6RewriteModules

anmonteiro23:08:02

I’ll try to come up with a fix and PR tonight

anmonteiro23:08:14

exciting to help move things along in Closure!

dnolen23:08:22

definitely 🙂

dnolen23:08:41

we’re officially the ones really pushing the boundary on this stuff 🙂