Fork me on GitHub
#cljs-dev
<
2017-07-05
>
Roman Liutikov00:07:38

@dnolen does compiler produce source map when compiling with modules? Here it says setting :source-map true should produce individual source maps for each module https://github.com/clojure/clojurescript/wiki/Compiler-Options#modules But when I set :source-map true I immediately get the following error:

Compiling "/projects/test/target/cljsbuild-main.js" failed.
java.io.FileNotFoundException: /projects/test/resources/public/js/compiled/out-min/cljs/core/constants.js (No such file or directory)
Works fine without source map setting

Roman Liutikov00:07:18

^ trying this from master

anmonteiro00:07:41

I’m guessing some weird interaction between modules and :optimize-constants

anmonteiro00:07:56

@roman01la can you try :source-map true :optimize-constants false?

Roman Liutikov00:07:46

@anmonteiro Will try tomorrow, going to sleep now. Thanks

anmonteiro00:07:09

let me try it locally

anmonteiro02:07:11

^ turns out this is not only a problem for :modules, but for the combination of :source-map and advanced compilation. Fixed in https://dev.clojure.org/jira/browse/CLJS-2169

Roman Liutikov07:07:46

anmonteiro: great, thanks for checking this!

anmonteiro02:07:34

^ the fix is quite simple. The bug was there all along, but manifested itself because source-on-disk was moved up

anmonteiro02:07:13

we were giving the constants JavaScriptFile a source-url when there really isn’t one so it would pass this check: https://github.com/clojure/clojurescript/blob/e86c36bb5408a0778bde6b56a292819d6d2af4c6/src/main/clojure/cljs/closure.clj?utf8=%E2%9C%93#L1701

thheller07:07:38

@anmonteiro IIRC goog.module is transpiled the same way ES6 would by using the embedded goog/transpile.js at runtime

thheller07:07:52

so probably not something we should ever emit

juhoteperi07:07:18

@anmonteiro I think that issue is about more supporting Closure libs that use goog.module, than changing what Cljs emits

bronsa09:07:05

I just had a build fail with 10:50:04 Caused by: java.lang.ClassCastException: clojure.lang.Var$Unbound cannot be cast to java.util.concurrent.Future

bronsa09:07:54

failure seems to be intermittent as subsequent build at same rev succeeded

bronsa09:07:14

is this a known issue? if not I'll log a ticket

bronsa09:07:38

this is the full stacktrace http://sprunge.us/FXMT

dnolen11:07:48

@bronsa yeah log a ticket

bronsa11:07:02

:thumbsup:

dnolen12:07:11

@juhoteperi left feedback, option 3 seems sound to me.

dnolen12:07:07

I also forgot that the transform fn takes compiler opts - I think this is fine for sharing whatever needs to be shared

juhoteperi12:07:38

@dnolen But transform fns can't update the opts?

juhoteperi12:07:07

Should they be able to?

dnolen12:07:13

@juhoteperi I just mean shared info, if they need to share state, they can write to the compiler state

dnolen12:07:20

I suppose the other way is for transform fns to return a map with :ijs :opts

dnolen12:07:29

(need better names)

dnolen12:07:52

it doesn’t really protect anyone from clobbering any more than just writing to compiler state though

bronsa12:07:56

opened https://dev.clojure.org/jira/browse/CLJS-2171, unfortunately I don't have a minimal repro for this (we've only hit this issue twice in the last ~10 days, over many builds)

dnolen12:07:16

@bronsa and no parallel-build involved here?

bronsa12:07:24

parallel build is enabled

bronsa12:07:59

I've pasted the compiler opts we're using in the ticket

juhoteperi12:07:05

I doubt there is much need for js-transforms to be able to modify the opts anyway. Probably reading ijs/opts is enough and using compiler-state or just a atom is enough for state.

dnolen12:07:21

this may again be what I suggested which is that we dont lock a require of cljs.spec.alpha \cc @anmonteiro

dnolen12:07:47

@juhoteperi yeah I’m inclined to agree

juhoteperi12:07:29

Okay, I think the specs for the implementation are quite clear to me now. I'll write version to use symbols + functions today, and I'll leave the multimethod around for backwards compatibility.

dnolen12:07:53

@bronsa I don’t know if it’s easy for you to test master in your setup but I locked require of cljs.spec.alpha in master

bronsa12:07:21

testing master is not the hard part, it's getting the bug to happen again unfortunately

bronsa12:07:02

i'll give master a spin anyway

mfikes13:07:14

^ The Lumo parallel build failure involved cljs.spec.gen.alpha/lazy-prims, so maybe this is the the same

potetm13:07:33

@dnolen I just added a comment to https://dev.clojure.org/jira/browse/CLJS-2168. I'm not sure if there will be anything more to discuss about it, but feel free to hit me up here if it'll save you a minute. I'll be in and out all day.

anmonteiro15:07:39

also, I’ll write something up about string-based requires to add to @mfikes blog post snippet for the next release

dnolen15:07:10

I’m actually thinking the next release will need at least 3 posts

dnolen15:07:28

1) aget/aset 2) :modules 3) :npm-deps + string based require

dnolen15:07:45

order matters less than, I think each of these deserve their own post

dnolen15:07:03

and the release post states that these are coming

dnolen15:07:06

and maybe release one same day

anmonteiro15:07:21

perhaps we can roll out posts before the actual release

dnolen15:07:30

that’s another option

anmonteiro15:07:31

to announce what’s coming

dnolen15:07:37

and I’m OK with that!

anmonteiro15:07:31

I’ll put something together for npm-deps + string requires

mfikes15:07:42

Here's the aget / aset draft. Knowing that it would be standalone, it could afford more content if there was some aspect we wanted to additionally mention, emphasize, etc. https://gist.github.com/mfikes/ba5e999eeef8295ee9d8d53af94ebc18

dnolen15:07:03

@mfikes can we please move that to a ClojureScript PR?

dnolen15:07:10

er clojurescript-site PR

mfikes15:07:31

It does not currently mention unsafe-get or detail goog.object/getValueByKeys, fir example. I'll put it in a PR where it is easier to review

mfikes15:07:19

On the previous post I had put the author as ClojureScript Team. We could decide for these posts whether that is continued, or if we put individual names on posts.

dnolen15:07:06

I think individual names is nicer to call out contributors

dnolen15:07:35

I think release notes ClojureScript team makes sense

anmonteiro15:07:38

@dnolen btw in the duplicate alias patch I don't check across require / require-macros on purpose. This is because of a cljs.core.async thing we noticed worked last year

dnolen15:07:55

yes I noticed that

mfikes15:07:30

Perforce, currently undated

mfikes15:07:45

@anmonteiro I tried your "string" require patch with re-natal—I think current setup there uses js/require a bit. My hope is to maybe dispense with that approach in favor of the new ns form capability. If so, I could better help validate this new feature.

anmonteiro15:07:56

Cool. I added an example to the ClojureScript repo too

anmonteiro16:07:32

@dnolen random thought that has been in the back of my head: given the amount of work that has been going into the ClojureScript compiler (and associated churn) what do you think about nightly releases (maybe through Maven snapshots)?

anmonteiro16:07:51

that way people who don’t know or want to build the compiler can try out things before they’re released

mfikes17:07:19

One related thought: We didn't seem to find bugs in the 1.9.665 release until it was cut. (Perhaps at least the "pre-release" pattern is useful.)

juhoteperi17:07:50

Travis supports triggering builds daily, so should be possible to configure this there

dnolen17:07:59

@anmonteiro that’s something we should sort out with Alex Miller I think

dnolen17:07:18

he’s on vacation, but when he returns we can bring it up

anmonteiro17:07:47

incorporating the CI work Mike has put together in the official repo would also be valuable IMHO

anmonteiro17:07:13

unless Alex has other ideas that include the current Clojure build system

dnolen17:07:09

CI is yet another discussion but that would probably not go very far - we have CI for Clojure

dnolen17:07:33

I think getting the existing CI to work for ClojureScript would be fine

dnolen17:07:52

and it would be great if we could build all dependents we maintain

dnolen17:07:58

core.async, test.check, etc.

dnolen17:07:18

thus why I think Nashorn test runner is important low hanging fruit

dnolen17:07:29

then Alex doesn’t have to go and sort out Node.js

dnolen17:07:54

all that said - I don’t think this means @mfikes Travis CI stuff isn’t useful

dnolen17:07:07

for example it would be great if that CI built community libs and tested them

dnolen17:07:15

Reagent/Om/Re-frame etc.

juhoteperi17:07:27

I wonder how hard it would be run tests inside a Docker container? Then it would be easy to maintain test environment for Cljs using Dockerfile at the repo

juhoteperi17:07:46

This is what I do with Cljsjs CI, I have Dockerfile with Java+Clojure+Boot+Node

mfikes17:07:34

If anyone is interested, you can now fork the ClojureScript repo, turn on Travis CI for it, and push potential patches to your fork and let the tests run there. I've been doing that lately (on an under-powered laptop).

anmonteiro17:07:36

@dnolen actually I hadn’t noticed that we have a .travis.yml file in the ClojureScript repo. Anyone with push access can set it up in http://travis.org

anmonteiro17:07:47

it’ll basically start building automatically

dnolen17:07:10

next step start build community libs against that and running tests

dnolen17:07:14

would be cool

juhoteperi17:07:59

Those tests can even run parallel so it should be possible to run them relatively fast

mfikes17:07:17

For now, I've been having the primary ClojureScript repo master branch commits pushed automatically over to https://github.com/mfikes/clojurescript where Travis runs the tests.

dnolen17:07:20

does having a clojurescript-oss org make any sense?

mfikes17:07:20

Would that somehow be useful for dependent libs. hmm... :)

anmonteiro17:07:49

I like the idea but what would you put there?

dnolen17:07:11

well I’m assuming we need travis scripts for OSS libs

dnolen17:07:20

we can collect them there?

juhoteperi17:07:06

If we use Travis or similar system, and the tests are triggered by cljs repo commits, it would be easiest to maintain them at the cljs repo

mfikes17:07:05

Om, for example, could have a .travis.yml in its own repo, But how could you get Om to run its tests for each ClojureScript commit / build?

juhoteperi17:07:08

regex replace on project.clj file 😕

anmonteiro17:07:33

I think what David has in mind is collecting some scripts to 1. build CLJS 2. clone those OSS projects 3. test them against pre-release CLJS

anmonteiro17:07:44

correct me if I’m wrong

anmonteiro17:07:19

that’s going to inherit failures from those projects too if they’re in a failing state

juhoteperi17:07:22

daily or even per commit performance/output size etc. benchmark + graphs would also be cool

dnolen17:07:40

@anmonteiro this why cljs-oss would just be travis build scripts

dnolen17:07:49

can you not depend on other builds in travis?

mfikes17:07:51

That would be extremely useful for catching subtle regressions—especially around funky optimization work.

juhoteperi17:07:27

@dnolen Nope, Travis is quite simplified, builds are only triggered by commits or timed

dnolen17:07:28

then we could also make a github static page which shows build state

dnolen17:07:47

@juhoteperi does CircleCI support this?

juhoteperi17:07:24

...though both Travis and Circle have HTTP endpoint that could be called by other project

juhoteperi17:07:29

that can trigger the build

mfikes17:07:34

So, if a lib wanted to participate, it would PR it's built-relevant bits to clojurescript-oss. That would be real nice!

dnolen17:07:46

@juhoteperi that seems sufficient

juhoteperi17:07:08

Well, we can start with just testing the setup manually and setting up the trigger later

dnolen17:07:52

cljs-oss or clojurescript-oss for org name?

mfikes17:07:08

Shorter better

mfikes17:07:03

So would the idea be that a .travis.yml clones ClojureScript, builds it, and then clones n other oss projects, and builds them against the just-built ClojureScript?

mfikes17:07:31

Or perhaps there could be multiple repos inside cljs-oss one per OSS project

dnolen17:07:43

I think you probably want one per OSS project

dnolen17:07:50

different projects may need different things

mfikes17:07:57

Makes sense. And the http endpoint mechanism can chain a ClojureScript clone & build based on a main repo commit, and each OSS repo can use the same mechanism to trigger their own.

mfikes17:07:26

One example: Planck is already set up to clone ClojureScript master, build it, and use it to build Planck, and then run its own test suite, all based on manually running its script/pilot script. It would be nice if this were automated inside cljs-oss as a repo

juhoteperi18:07:05

So one repo to build Cljs, and push it somewhere for oss project tests to use?

dnolen18:07:58

or people can just submit Travis scripts for building against master

dnolen18:07:09

so they don’t have pollute their own repos w/ this detail

juhoteperi18:07:47

Then each project needs to build Cljs again? Probably that is fine also

dnolen18:07:33

if that can be avoided - better

juhoteperi18:07:43

If each OSS project has it's own repo under cljs-oss

juhoteperi18:07:54

each with their own travis script

dnolen18:07:08

they should pull down the build jar

dnolen18:07:14

not rebuild

juhoteperi18:07:26

okay, but where do they pull the jar from?

dnolen18:07:36

wherever we want to put it 🙂

juhoteperi18:07:59

okay, that was what I was saying 😄

anmonteiro18:07:51

I still think this ticket is valuable for specifying a :file-min JS module to be processed under advanced optimizations

darwin18:07:24

sounds good, but as the first iteration I would make it simpler, just one repo “cljs-oss/clojurescript”, which would on trigger do all the work at once, e.g. build new jar file and trigger all participating OS projects, links to results would be embedded directly in the git commit message (e.g using travis badges), so no need for collecting them into results repo

anmonteiro18:07:30

very useful in the React case for example, where they have different bundles for DEV and prod (this way we can provide a simple way for people to consume these modules without having to muck around with fake process.env namespaces)

juhoteperi18:07:09

@darwin so do you mean OS projects wouldn't have separate repos, or just the the output would not be commited to a repo?

darwin18:07:43

yes, both, all maintainers of those “participation scripts” would collaborate together on a single repo, also because those scripts will likely trigger long-running tasks somewhere on the web (e.g. travis), you would just commit a list of links into the git message

darwin18:07:48

without waiting

dnolen18:07:51

fwiw, I don’t think having the main thing do this is a good idea even in the first cut

dnolen18:07:11

individual libs might want to different completely unrelated kinds of things

dnolen18:07:04

the overhead doesn’t seem interesting to me at all either

dnolen18:07:10

people can sort out their own build

juhoteperi18:07:56

separate repos allows for e.g. only reagent test be triggered when there is commit to reagent repo, instead of all the projects being tested

juhoteperi18:07:28

(though there are magical ways to avoid that... e.g. cljsjs CI scripts uses clojars API to check which packages have been deployed :D)

darwin18:07:55

ok, I see your point, I wanted to avoid managing tons of repos which will have similar code in them

darwin18:07:20

most repos will simply trigger travis build on their main repo with some custom env variable set to use specific clojurescript jar

darwin18:07:24

I can imagine

juhoteperi18:07:57

ah, my idea is that the tasks clone the main repo, change the clojurescript version on project.clj/build.boot and run the test there

juhoteperi18:07:14

and no triggers back the main projects

juhoteperi18:07:34

that will duplicate some code that is present in OS project main repos, but it will make easier for us to do something with the results, e.g. push to a result repo

darwin18:07:32

ok, but then I as a maintainer of libraryX will have to think about cljs-oos/lib-x and potentially update the thing whenever I touch travis-related stuff

juhoteperi18:07:22

hmm, touching the travis-related stuff (travis.yml) at main repo wouldn't affect cljs-oss/lib build?

juhoteperi18:07:45

though changing the test runner or such would indeed cause need to update cljs-oss/lib

juhoteperi18:07:37

I'll add a note about triggering build at the main repo with custom env variables, that could work, at least for those cases that run Travis/Circle

darwin18:07:09

correct, I meant the test-runner stuff, but there are maybe some deps which are described in readme for developers who want to hack on the lib and test it, but this info is somewhere in .travis.yml in before_install or similar sections

darwin18:07:34

I would have to keep them in sync with cljs-oos/lib, because that bootstrapping must be done there as well

juhoteperi19:07:04

triggering builds over http endpoint requires API token for the target Travis project, will have to see how hard that will be configure for each project

darwin19:07:41

that could be in travis secrets, that would the only thing someone super-admin of clojurescript repo could add

juhoteperi19:07:11

If cljs-oss/clojurescript is going to trigger OS project builds, we'd need token for each project

darwin19:07:47

ok, I was speaking about the mono-repo situation and assuming the whole job will be done by travis, sorry, in multi-repo as you suggested, that probably has to be in secrets somehow, you don’t want random people on internet triggering your ci builds 🙂

darwin19:07:21

e.g. if each of those project repos used travis, it can be a secret env encrypted in that particular repo for particular master repo

juhoteperi19:07:50

but would the mono-repo run all the tests (and contain test script for every project) or trigger builds in other projects?

darwin19:07:18

my current mono-repo idea, three branches master, builds, results : 0) builds are triggered by committing to builds branch with clojurescript SHA to be tested 1) uses docker to provide well-defined runtime environment 2) inside docker run a worker.clj script which accepts as a parameter SHA of clojurescript repo 3) this script will clone the clojure/clojurescript repo, build the jar and upload it somewhere (e.g. S3) 4) then there will be a list of libraries, each having their own job function, worker will call each of them to return some result (edn) 5) a job function for a project can do anything, but typical job function will trigger a build on its main repo (using travis secrets for access) 6) worker collects results from all jobs and prepares a commit message, this commit message will list all links, badge icons or whatever and commit it into results branch of the same repo

juhoteperi19:07:13

in this idea, new build is not triggered by changes to a library repo?

darwin19:07:20

inspiration 1: here I have a docker file which tries to resemble travis-ci, I use it to run my travis scripts locally: https://github.com/binaryage/dirac/blob/master/test/docker/Dockerfile https://github.com/binaryage/dirac/blob/master/docs/hacking.md#running-tests-via-docker

darwin19:07:30

@juhoteperi correct, I didn’t see that as a requirement, new clojure/clojurescript commits should trigger cascade of tests

juhoteperi19:07:36

Hm, it would be useful in cases where lib is broken and is fixed. But in most cases the libs are correct and Cljs shouldn't break them.

darwin19:07:09

inspiration 2: here I have a health-check branch which triggers builds (I imagine builds to be similar to this), the commit message encodes specific versions for the build: https://github.com/binaryage/dirac/commits/health-check

juhoteperi19:07:18

It would also give faster feedback if something that is done to a lib is broken with new cljs

juhoteperi19:07:25

But probably not necessary

darwin19:07:38

I see your point, let me think about it

darwin19:07:01

in #2 of my list, the worker.clj script could be parametrized with more options, for example in addition to clojurescript SHA, with filter of jobs to run (e.g. a regex), library authors wanting to re-test their lib for hot-fixes could trigger it maybe?

juhoteperi19:07:09

But libs could check for this themselves, by always running tests against their default Cljs and then against latest cljs-oss jar

juhoteperi19:07:13

In that case we wouldn't get result back to cljs-oss

darwin19:07:26

I think this can be communicated via github, each commit with broken stuff can have discussion, and lib authors can singnal that they fixed their libs

darwin19:07:41

and it will be fixed on next trigger (or new commit) in cljs repo

darwin19:07:44

I can imagine there will be broken things even not as a cause of cljs repo changes, tests might be flaky or some env problems

darwin19:07:16

so broken tests are not something fatal, it will just signal clojurescript team that something didn’t go as expected and someone should take a note

juhoteperi19:07:02

Well, you should probably also have access to cljs-oss org so I sent you an invite

darwin19:07:32

thanks, I’m in

darwin19:07:52

in #4 the job functions can be a) shell scripts (for maximal flexibility) b) clojure functions (for convenience) - I assume most people will create their own clj (or sh) file and work on their own thing there, in clj case they could maybe reuse some helpers provided in some shared place

dnolen19:07:42

@anmonteiro left a question about CLJS-1822

darwin19:07:47

if someone wanted to do something really heavy/crazy in their job function we would recommend to do it outside of this worker, maybe triggering their own http hook, travis build somewhere else

darwin19:07:19

in #6 I would think that result edn will have just simple textual description for now, but later it could have some well-specified data (e.g. timings)

anmonteiro19:07:26

@dnolen right, no implicit magic there. We only use :file-min if the user specified an explicit override

anmonteiro19:07:59

I added a test case that demonstrates this

juhoteperi19:07:04

@anmonteiro Did you test es6 module support after updating Closure? I'm having problems testing preprocessing, looks like babel is running okay, but looks like es6 module processing doesn't do anything

anmonteiro19:07:36

Oops we may have to back that out

anmonteiro19:07:53

@juhoteperi I'll have a look in a little bit

anmonteiro19:07:03

And write a regression test

juhoteperi19:07:10

I'll check with older Closure version to see if that helps

anmonteiro19:07:19

Do you have a simple example that I can use?

juhoteperi19:07:34

Works with v20170521

anmonteiro19:07:16

Hah, nice. We definitely need tests for that stuff, so I guess this is a good opportunity

juhoteperi19:07:45

Just calling convert-js-modules with mocked js-module map (with source as string) should be enough

juhoteperi19:07:59

Hmm I thought I even wrote something like that..

juhoteperi19:07:27

Ah module_processing_tests only checks :commonjs

juhoteperi19:07:44

Should be easy to extend that with another module which uses :es6

anmonteiro19:07:19

Yeah, I know we have some for commonJS but I guess we need to exercise the ES6 code now that it's becoming more widespread

dnolen19:07:48

@anmonteiro I left another comment on CLJS-1822, lets keep the discussion there so it doesn’t get lost in the Slack aether

anmonteiro19:07:55

They rewrote the ES6 pass in Closure for this release, we may have to change something on our end

anmonteiro19:07:58

Answered both questions in the ticket

juhoteperi20:07:47

Re: preprocess symbol support, should Cljs catch exceptions from require and show warning or throw exception and stop the build?

juhoteperi20:07:41

Warning and continue would probably be most similar to current logic if the multimethod is not found

dnolen20:07:21

@anmonteiro thanks ok, makes sense will review one more time

juhoteperi20:07:48

But catching the exception would probably hide useful information about debugging the problem...

dnolen20:07:05

put the :cause in there

dnolen20:07:42

@juhoteperi also make sure to use load-mutex to lock the require

dnolen20:07:38

if you want to make a reusable require helper that does this - also cool with me

juhoteperi20:07:15

So I should throw ex-info with cause and additional information?

dnolen20:07:47

meaningful error string, original :cause

juhoteperi20:07:41

Hm, I should probably improve the error message a bit more

juhoteperi20:07:11

The warning messages could also be more specific about the problem, multimethod method not found, symbol doesn't point to a var/function, preprocess value is something else than keyword or symbol

juhoteperi20:07:28

One thing I was thinking about: Is processing files one by one good for all use cases? Are there some tools that need to process all the files in one go, like Closure CJS/ES6 processing?

dnolen20:07:39

to be honest I just can’t think of case where this is desirable

dnolen20:07:28

or even where in the JS world people do this - cross file analysis isn’t much of thing

dnolen20:07:00

@juhoteperi agree about better error messages here

juhoteperi20:07:52

What's the best way to add context to the warning (should there be single warning type or multiple?), add info the the extra map and use that in error-message multimethod?

dnolen20:07:40

@juhoteperi I don’t think we need to get fancy here, just assert and throw or wrap and rethrow w/ info if important

dnolen20:07:52

basically same as how we handle opts validation

dnolen20:07:00

@anmonteiro ok left one more comment on 1822

anmonteiro20:07:34

@dnolen hrm I just copied the tests over from what @juhoteperi had

anmonteiro20:07:39

let me try to use with-redefs

anmonteiro20:07:26

@dnolen attached new patch to CLJS-1822 that uses with-redefs

dnolen21:07:58

poking around I don’t see an obvious reason why Closure ES6 module processing would have stopped working

dnolen21:07:05

we set lang in/out and this looks OK

anmonteiro21:07:06

there might be more coming

juhoteperi22:07:02

@dnolen I atteched a patch with broken test case

juhoteperi22:07:09

Works with previous Closure version

dnolen22:07:04

@juhoteperi patch is missing es6_hello.js ?

dnolen22:07:14

yes I’m discovering this by looking at the code 🙂

juhoteperi22:07:02

options.needsTranspilationOf(FeatureSet.Feature.MODULES) should be true as input featureSet has MODULES but output doesn't have

juhoteperi22:07:49

Oh, the ES6 transpilation doesn't happen at parse stage anymore

juhoteperi22:07:00

> This moves it from happening at the end of parsing into DefaultPassConfig, where it can be more easily reordered around other passes.

juhoteperi22:07:13

And Cljs code only calls parse

dnolen22:07:34

yeah I have it sorted here

juhoteperi22:07:15

@dnolen report-failure should probably be called after parse to check for parse errors?

juhoteperi22:07:10

And if there are problems during parsing, module rewrite probably wont succeed

anmonteiro22:07:20

we should also probably reuse the get-js-root function

dnolen22:07:36

get-js-root doesn’t do exactly what we want here though

anmonteiro22:07:45

right I saw that

dnolen22:07:46

root has two nodes, first is externs next is the scripts

anmonteiro22:07:54

it does get the scripts child though

anmonteiro22:07:09

that was just a nitpick in any case

dnolen22:07:09

heh yeah, we might want to think a bit about making some sensible wrappers in front of all this imperative stuff

dnolen22:07:32

I see that Closure itself has gone down this route via pass factories, parseInputs which returns root etc.

dnolen22:07:58

but not for this commit 🙂

juhoteperi22:07:08

And if later want to support es7 features etc. we'll need to support other passes than Es6RewriteModules

dnolen22:07:33

actually there’s nothing to do there 🙂

dnolen22:07:45

the only reason we just rewrite modules here is to get a GCL namespace

dnolen22:07:59

advanced compilation will take care of all that other stuff

juhoteperi22:07:00

oh right, and then the ES7/8 features can be processed later

dnolen22:07:21

this means we can get hot loadable JS files ASAP

dnolen22:07:26

w/o all the other stuff slowing things down

anmonteiro22:07:04

re: CI I think we should also set up AppVeyor

anmonteiro22:07:19

would make our Windows support first class

juhoteperi22:07:06

Yeah. Boot appveyour config could be helpful, it uses mingw for make and bash and I think there is grep available there, so tests could be run similar to travis config: https://github.com/boot-clj/boot/blob/master/appveyor.yml

dnolen23:07:02

@anmonteiro I like that idea - our Windows support has been spotty having CI catch obvious bad stuff would be huge

juhoteperi23:07:40

Previously preprocess multimethod could work with any values, e.g. strings, should that be still supported or can we presume it was only used with keywords?

juhoteperi23:07:51

Hmm, calling the multimethod always if the value is not symbol is even simpler than checking for keywords

dnolen23:07:19

we never said we supported anything but keywords, and it’s an alpha feature

dnolen23:07:15

and we may want to dump the multimethod later, so better just to close the scope

juhoteperi23:07:50

@dnolen report-failure should probably be called both after pars and after Es6RewriteModules, because the latter can also report problems: https://github.com/google/closure-compiler/blob/d24d175572ebf97b06e480f04c3f452bb6355afa/src/com/google/javascript/jscomp/Es6RewriteModules.java#L268

juhoteperi23:07:31

Hmm, in fact I'm not sure how those compiler reports work, are parse errors present even after Es6RewriteModules...

juhoteperi23:07:09

Looks like nothing will clear those errors, so I was mistaken and calling report-failure the last thing should be enough

juhoteperi23:07:53

But perhaps hasErrors should be called between parse and Es6RewriteModules, that is what Closure does: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/Compiler.java#L722-L737