Fork me on GitHub
#cljs-dev
<
2017-07-15
>
mfikes00:07:57

Sweet, one way to judge the success of António’s “ClojureScript Is Not An Island” post is that it made it into the top 10 Hacker News “ClojureScript” posts of all time

anmonteiro01:07:26

Hrm I suppose that 2nd analyze call might be needed https://dev.clojure.org/jira/browse/CLJS-2242

anmonteiro01:07:38

I wonder if we're just not loading macros on the first call or something

mfikes13:07:50

Summary of checked arrays for downstream Figwheel use: - Patch landed there fixing its misuses: https://github.com/bhauman/lein-figwheel/commit/48eba2f7e481be4079fdfc4b606d3eb52217a891 - Patch exists for core.async (which Figwheel uses) https://dev.clojure.org/jira/browse/ASYNC-201 This should be sufficient for mainstream use of the feature. The only remaining issue is that Figwheel transports *print-err-fn* args using edn https://github.com/bhauman/lein-figwheel/issues/575

dnolen16:07:02

closing in on comprehensive support for all JS module types at the REPL in non-Node.js context

anmonteiro16:07:27

@dnolen let me know if there are any areas you think may be flaky and could use help with manual testing

dnolen16:07:22

I think all the obvious issues are solved now - I left a lot comments on the trail

dnolen16:07:30

later we probably want to rethink how we do this

dnolen16:07:47

I’m testing this under Nashorn/Rhino right now

anmonteiro16:07:20

I'll be really surprised if those just work

dnolen16:07:38

I’ll be mad if they don’t just work since we spent a lot of time on this before 🙂

dnolen16:07:28

Nashorn just works

anmonteiro16:07:37

I think we might be done wrt. self-hosted too

anmonteiro16:07:28

The global exports patch you applied this morning makes every new feature work downstream in Lumo

dnolen16:07:36

Rhino just works

dnolen16:07:02

This should mean all downstream REPLs (Figwheel etc.) now get this functionality for free

dnolen16:07:08

nothing special for them to do

dnolen16:07:28

all JS modules types can be hot-loaded

anmonteiro16:07:34

That's awesome

anmonteiro16:07:15

I wonder how many JS people out there really understand what we have going on here wrt tooling 😛

dnolen16:07:13

not many 🙂 but we should market that part better

anmonteiro16:07:49

Hopefully the blog is a good start

dnolen16:07:30

ok master is in a good state far as I can tell, tests passing, and I couldn’t break loading JS modules in my simple testing

dnolen16:07:55

there’s probably some finessing we could do - but everyone should try it out and bring feedback

anmonteiro16:07:15

How hard do you think it should be do add some REPL tests?

anmonteiro16:07:27

At least in a Node env

anmonteiro16:07:46

Maybe later use Chrome headless for browser repl now that it's a thing

dnolen16:07:55

hrm, probably not hard if we just setup some infrastructure first

dnolen16:07:05

I don’t think we need to bother with browser REPL actually

dnolen16:07:12

Nashorn + Node.js would be good enough

dnolen16:07:19

Browser/Nashorn/Rhino code path is all the same

dnolen16:07:27

Node.js is separate because of :nodejs target

anmonteiro16:07:34

I bet you feel this pain more than I do but we've broken REPLs on several occasions without knowing

dnolen16:07:17

yeah, automating REPL testing would be really sweet, it’s just an really easy step to miss

dnolen16:07:24

there’s an existing ticket for it

dnolen16:07:42

I don’t think it will be that hard

dnolen16:07:50

REPLs are just over standard in/out

anmonteiro16:07:06

Cool, I may look into it in the near future

dnolen16:07:18

just needs some scaffolding to make it easy to write test cases for

anmonteiro16:07:38

Testing has also been a theme for this release and I'd love to keep it that way

mfikes16:07:56

One nice side effect of that particular test env is it is an older JSC

dnolen16:07:43

heh just realized :npm-deps should work just fine for Ambly too 🙂

mfikes16:07:00

Whoa. Mind blown

dnolen16:07:19

I’ve been testing with {:npm-deps {:lodash "4.17.4"}} that’s all you need to do w/ your REPL options

anmonteiro16:07:17

There has to be a way to fix that

anmonteiro16:07:54

Not without some fiddling I imagine

mfikes16:07:05

It is the wild west

anmonteiro16:07:34

We'd need to make it aware that React is Closure's React

anmonteiro17:07:01

Maybe through a CommonJS shim or something

dnolen17:07:23

hrm I don’t think so

mfikes17:07:32

We actually might be extremely close to RN working with all of this. It is a Rumsfeldian issue right now.

dnolen17:07:42

far as I know you just need a custom module loader

dnolen17:07:17

I think if you had a module loader that understood Facebooks weird JSDoc @provides thing you’d be good

anmonteiro17:07:57

Hah didn't know they still used that

dnolen17:07:05

but actually … maybe not even?

dnolen17:07:20

one thing to realize is that we refuse to get into the relative include madness

dnolen17:07:45

but yeah somebody needs to mess around with it

mfikes17:07:11

For me to contribute, I have to first understand what re-natal is doing to make things work. I used to understand natal / Ambly.

anmonteiro17:07:18

My mental model is that we may have 2 copies of React if we're processing it and the Packager is loading another copy from node_modules

dnolen17:07:35

right I’m suggesting forget Packager

anmonteiro17:07:02

Not sure if you can do RN without the Packager

anmonteiro17:07:20

Hrm but I saw something that might be of interest the other day, 1 sec

dnolen17:07:28

you will need a custom view with your own loader thing

dnolen17:07:41

but getting rid of Packager never seemed itself be a real problem

dnolen17:07:53

custom view with your own loader is more annoying to do

anmonteiro17:07:56

^ I think this replaces the pckager

anmonteiro17:07:07

And might be easier to integrate with

mfikes17:07:13

Right, someone in #cljsrn mentioned that thing. I’m starting to feel the tool fatigue 🙂

anmonteiro17:07:34

Yeah this might be a better conversation for that channel

mfikes17:07:36

If it solves the problem, I’m all for it 🙂

mfikes17:07:09

Also, perhaps Expo will solve the problem if there is money

dnolen17:07:44

@anmonteiro that package seems to imply this would be feasible now?

dnolen17:07:48

no need for custom view

mfikes17:07:55

Where we stand right now in RN land is a bit fragile. It barely works, and whenever Facebook releases, it stands a good chance of breaking.

dnolen17:07:47

heh wow Figwheel error reporting has really gotten good

dnolen17:07:07

testing Figwheel :npm-deps now

mfikes17:07:25

Yes. Right after we added that Levenstein distance suggestor in ClojureScript, Bruce out did it by a mile

dnolen17:07:00

Figwheel just works

mfikes17:07:38

@anmonteiro One thing I’ve noticed, if I try to use :npm-deps, my package.json gets updated so that the line reading

"react": "16.0.0-alpha.6",
gets turned into
"react": "^16.0.0-alpha.6"

mfikes17:07:08

I’m going to switch away from yarn to just npm. Maybe that is exacerbating things.

anmonteiro17:07:15

I don’t think it is

anmonteiro17:07:39

that’s probably the result of doing npm install [email protected]....

anmonteiro17:07:51

@dnolen do you think we should npm install --no-save?

anmonteiro17:07:03

that wouldn’t save deps in package.json

mfikes17:07:11

I have this, FWIW:

:npm-deps       {:react "16.0.0-alpha.6"
                          :react-native "0.44.2"}

anmonteiro17:07:48

I don’t know what the most sensible default would be

mfikes17:07:22

The background of this: If you set up a new app using re-natal and it sees that you have yarn installed, it sets things up with a yarn.lock file, etc.. I’m starting from that working state, and adding :npm-deps as above and seeing if I can then make use of it.

anmonteiro17:07:21

the problem of using :npm-deps & yarn is that one will overwrite the other

anmonteiro17:07:39

like, there’s no way you can use the react you installed from yarn if you specify a new one in npm-deps

mfikes17:07:08

yeah… I’ll simplify by removing yarn for the time being

dnolen17:07:35

@anmonteiro I think current behavior is OK as default + we need a flag?

anmonteiro17:07:55

flag for what?

dnolen17:07:27

I don’t really care that much what the default is, just that we choose one

dnolen17:07:31

and have a flag to set

anmonteiro17:07:34

here’s a different suggestion: a flag that doesn’t install modules, but uses a node_modules installation if it finds one

anmonteiro17:07:51

then people can use yarn if they please

anmonteiro17:07:21

then we provide the blessed :npm-deps path which saves by default

anmonteiro17:07:31

and people that want more control can tweak their behavior

anmonteiro17:07:08

I quite like the idea, let me prototype something

anmonteiro18:07:42

@dnolen should be OK to overload :npm-deps to accept true too, or would you prefer a new compiler option?

anmonteiro18:07:10

hrm the problem would be upstream npm-deps

anmonteiro18:07:41

maybe this could really be a big hammer in that you’d need to specify overrides for those upstream deps too

anmonteiro18:07:54

I can see that being useful for people that know what they’re doing

dnolen18:07:09

I don't think we need to do that now. Separate option please since :npm-deps is a deps.cljs thing

anmonteiro18:07:06

surprisingly clean now that we can index the top level node_modules directory

anmonteiro18:07:18

then users can use whatever build tool they want

anmonteiro18:07:25

@mfikes mind giving this patch a try?

anmonteiro18:07:11

simply add

{"devDependencies": 
  {"module-deps": "*",
   "resolve": "*",
   "browser-resolve": "*"}}

anmonteiro18:07:18

to your package.json

anmonteiro18:07:27

along with your other deps

anmonteiro18:07:45

then replace the :npm-deps entry in your compiler options with :node-modules true

anmonteiro18:07:23

it should now respect your yarn.lock file

anmonteiro18:07:40

(make sure you install the dependencies yourself with yarn install)

mfikes19:07:43

@darwin Is Canary actually building master now? (The version numbers look accurate. 🙂 )

darwin19:07:23

@mfikes almost there 🙂 in general it should be able to build any fork you point it to

darwin19:07:23

@mfikes maybe you could look at env it is passing to project travis builds and prepare planck: https://travis-ci.org/binaryage/cljs-devtools/jobs/253987756/config

mfikes19:07:52

I suppose I can mess with Planck’s .travis.yml — if it is building in that particular environment, I’d need to sort out the fact that it does language: c and has osx and linux-specific setups

darwin19:07:52

wait, I want to trigger builds in your CI, I think there is no need to change anything, just make sure that CANARY_CLOJURESCRIPT_VERSION gets used

mfikes19:07:09

(Planck has a build matrix that fans out to 4 separate things, if you include gcc vs clang)

mfikes19:07:24

Oh, OK, easy enough

mfikes19:07:36

I’ll push in a change that looks for that env var

darwin19:07:51

ok, we can pass in some travis params to build only one of those builds in matrix

darwin19:07:10

this is another thing you should look at, you need to pull that jar https://github.com/cljs-oss/canary/blob/master/scripts/install-canary.sh

mfikes19:07:23

That was my next question 🙂

mfikes19:07:39

OK, so if I add that line to Planck’s before install it would take care of getting the JAR?

darwin19:07:58

and it will be future-proof

mfikes19:07:12

Ahh, and I see you can do def in project.clj… one sec and I’ll push that stuff in 🙂

mfikes19:07:28

@darwin Planck may be ready for Canary now

darwin19:07:15

great, let me add it as a second task and test it

darwin19:07:35

do you want to run full matrix or want only specific build?

mfikes19:07:21

I think a specific build is sufficient for these purposes. You could pick Linux, clang

mfikes19:07:23

How will you trigger a Planck build? Do you need a token from me?

darwin19:07:28

@mfikes one last thing, will need CANARY_PLANCK_TRAVIS_TOKEN encrypted in https://github.com/cljs-oss/canary/tree/jobs

darwin19:07:31

.travis.yml

darwin19:07:59

travis encrypt CANARY_REPO_TOKEN=xxx -a env.global

darwin19:07:25

btw. it is nice that you don’t have to tell me, you will encrypt it for travis eyes only

mfikes19:07:51

So, I’ll get a token, substitute it in for xxx above, and then what do I do with the result?

darwin19:07:04

change also the env var name

darwin19:07:21

and run it in jobs checkout, it will modify the .travis.yml file

darwin19:07:25

then commit

darwin19:07:37

ideally with [ci skip] so you don’t trigger canary

darwin19:07:54

look at some other “config” commits in that branch

mfikes20:07:51

@darwin By “change also the env var name”, should I change CANARY_REPO_TOKEN in the command above to, perhaps PLANCK_REPO_TOKEN?

darwin20:07:05

to CANARY_PLANCK_TRAVIS_TOKEN

mfikes20:07:46

OK, so it indicated

Detected repository as mfikes/planck, is this correct?
I confirmed. It appears to have hopped over into my local Planck directory and modified the .travis.yml there

darwin20:07:34

that’s wrong, you have to checkout canary jobs branch and do it there

darwin20:07:43

this is for canary’s .travis.yml

mfikes20:07:18

Ahh PEBKAC

mfikes20:07:42

@darwin It is wanting to make changes that involve stripping off quotes. I’ll make a gist…

darwin20:07:22

maybe you have some other travis cli tool version than I do? they also try to reformat .travis.yml

darwin20:07:53

mine is 1.8.8

mfikes20:07:09

$ travis --version
1.8.8

darwin20:07:22

hmm, that stripping of quotes look ok to me

mfikes20:07:26

I’m on macOS

darwin20:07:32

me too, feel free to commit it

darwin20:07:45

add [ci skip] into commit message

mfikes20:07:59

Just [ci skip] or do I need to add jobs info?

darwin20:07:44

“add CANARY_PLANCK_TRAVIS_TOKEN into env vars [ci skip]”

darwin20:07:56

this won’t trigger canary job

darwin20:07:01

this is just a configuration tweak

mfikes20:07:12

My commit message should literally be that? Without the quotes?

anmonteiro20:07:16

@mfikes FWIW [ci skip] or [skip ci] is a Travis thing, not canary

juhoteperi20:07:26

I wondered a bit why CLJS-2245 was necessary, because using local node modules worked before, but I see CLJS-2240 changed code so that node modules were indexed only when :npm-deps is given

darwin20:07:29

now when you are at ti, you may try to trigger a new canary job

darwin20:07:34

git commit -m "job --compiler-repo --compiler-rev canary" --allow-empty

mfikes20:07:25

Change anything there to point at Planck, or literally that?

anmonteiro20:07:30

@juhoteperi right this is about making it explicit. It would shell out even if you didn’t want it to 😛

darwin20:07:02

but it will build whole matrix now

darwin20:07:17

haven’t figured out how to exclude some configs from matrix

darwin20:07:52

will probably overwrite your compiler and os config keys without studying matrix.include stuff

mfikes20:07:22

I don’t see any Travis jobs running. Is this supposed to trigger one?

juhoteperi20:07:30

@anmonteiro Hmm, it previously would only shell out to node to resolve dep graph, not npm

darwin20:07:31

you have to push the commit

mfikes20:07:13

I pushed the first commit. But I’ve not done

git commit -m "job --compiler-repo  --compiler-rev canary" --allow-empty
Is that what you literally want me to do @darwin? (Followed by a push?)

juhoteperi20:07:35

But it is fine it is now explicit. Just wanted to be sure to understand the changes

anmonteiro20:07:10

oh hrm you’re right

anmonteiro20:07:16

I might have missed that

anmonteiro20:07:36

I think we’ll have the same behavior if we revert both CLJS-2240 and CLJS-2245

anmonteiro20:07:53

because it detects node-required anyway

darwin20:07:18

@mfikes I’m not sure if clojurescript master would work now, did the testing on my fork with some patches applied, for master it would then be just git commit -m "job" --allow-empty or git commit -m "job -r SHA" --allow-empty

anmonteiro20:07:13

I forgot to commit an untracked directory

anmonteiro20:07:25

I’ll fix in CLJS-2246

mfikes20:07:43

Sweet! Canary is building Plank now 🙂

darwin20:07:22

@mfikes the main job already finished with this report: https://github.com/cljs-oss/canary/tree/results/reports/job-000044-1.9.807-f63ab74 btw. canary just triggers the travis builds but does not wait for them, that is probably the last major thing to implement

darwin20:07:10

and btw. I realized we cannot use http://shields.io badges or travis badges, they show just most recent build status

darwin20:07:30

at least I didn’t find a service which would provide icons for specific builds

juhoteperi20:07:35

Yes, this should work. I'll leave it to you and David to decide if this or additional option is better choice.

mfikes20:07:14

Planck build passed. 🙂 Will try with master now.

darwin20:07:31

@mfikes ok, go for it 🙂

darwin20:07:49

docker image is cached now, so it worked

mfikes20:07:59

This is so cool!

mfikes20:07:46

Canary built ClojureScript master, put a JAR at a URL that other repos can get, and triggered Planck to buiild with that JAR.

darwin21:07:54

@mfikes didn’t find a way how to limit travis api request to launch only specific build from matrix, their docs are not very specific about this: https://docs.travis-ci.com/user/triggering-builds and google didn’t help, tried this https://github.com/cljs-oss/canary/commit/67ff99da0c5da9e711bbab7c0edf47c75b8fcd0f but had to revert it back, it didn’t work

mfikes21:07:12

OK, either way is fine.

darwin21:07:24

let’s leave this open for now, it is not that important to be solved ATM

dnolen22:07:22

@anmonteiro lein test has an error for me on master, specifically test-node-modules-cljs-2246 has an exception

anmonteiro22:07:57

@dnolen you need to install yarn

anmonteiro22:07:12

or we could revise the test to not use yarn I guess

anmonteiro22:07:20

actually that might be better

dnolen22:07:22

yeah I don’t think the test should rely on yarn

dnolen22:07:32

there’s enough requirements as is

dnolen22:07:31

great thanks

dnolen22:07:57

on a plane to Berlin now 🙂

anmonteiro22:07:25

getting some time to hang before the conf

dnolen22:07:51

will try, doing some Cognitect client work before the conf

dnolen22:07:14

@anmonteiro tests still failing for me after the patch

anmonteiro22:07:32

passing locally, what did I do wrong

anmonteiro22:07:41

CI also passes

anmonteiro22:07:11

do you have a package-lock.json file or something?

dnolen22:07:31

do not but I had a package.json

anmonteiro22:07:37

something that may be preventing module-deps from being installed

dnolen22:07:21

trying again after deleting package.json will paste what I see if it fails again

dnolen22:07:38

(hoping to QA :global-exports at the REPL after I get past this)

anmonteiro22:07:52

just re-ran lein test again and no failures

dnolen22:07:10

already noticed some other broken things that need tests (:foreign-lib local overrides don’t appear to work anymore)

anmonteiro22:07:23

and I don’t have any untracked files

dnolen22:07:24

@anmonteiro still not working for me

anmonteiro22:07:10

lein test :only cljs.build-api-tests/test-node-modules-cljs-2246

anmonteiro22:07:19

^ does that succeed?

dnolen22:07:08

trying that - removed any random testing stuff first

dnolen22:07:46

@anmonteiro running just that one worked

anmonteiro22:07:28

@dnolen ls $TMPDIR | grep 'test-out$' | xargs rm -rf

anmonteiro22:07:34

then lein test again? 🙂

dnolen22:07:31

trying that

anmonteiro22:07:27

well that’s not deleting the folders for me

anmonteiro22:07:33

I might have given you a botched command

dnolen22:07:45

that’s ok I think I cleaned it up - trying lein test again

anmonteiro22:07:05

this is the actual command fwiw: cd $TMPDIR && ls $TMPDIR | grep 'test-out$' | xargs rm -rf

dnolen23:07:02

lein test still doesn’t work for me after manual clean

dnolen23:07:09

trying your command now

anmonteiro23:07:47

do you have a dirty worktree?

anmonteiro23:07:03

I’m out of ideas here

anmonteiro23:07:45

the patch applied, right?

dnolen23:07:45

not other than your patch

anmonteiro23:07:11

@dnolen can you push it to master and let Mike’s CI run it?

dnolen23:07:10

well I want to make sure I can get back to a sensible state too

anmonteiro23:07:48

node -v
npm -v

anmonteiro23:07:57

can you run those so I can try to repro?

dnolen23:07:31

master is just broken for me now

dnolen23:07:39

and the patch is just a different broken thing

anmonteiro23:07:41

I’m on 8.1.4 and NPM 5

anmonteiro23:07:48

so that’s probably why

anmonteiro23:07:52

installing those

anmonteiro23:07:10

I’ll modify the patch to make CI run Node 6 as well

anmonteiro23:07:15

so we can prevent further failures

dnolen23:07:03

I need to get master back to sensible state for myself - so first I’m probably going to comment out 2246 test

dnolen23:07:14

and push that until this can get sorted out so I can focus on other things

anmonteiro23:07:19

OK reproed your failure

anmonteiro23:07:33

feel free to comment that test

anmonteiro23:07:39

I’ll rebase on top of master when I figure this out

dnolen23:07:42

I think CI running the the lower bound of Node.js releases is generally a good idea

dnolen23:07:05

not really interested in bleeding edge Node.js stuff other than making sure it also isn’t breaking

dnolen23:07:47

makes me wonder how anybody gets anything done with that stuff

anmonteiro23:07:55

agreed we should be on LTS

anmonteiro23:07:00

which is 6 currently

dnolen23:07:37

tests appear to be passing if I comment out 2246, will confirm shortly

anmonteiro23:07:22

problem seems to be NPM

anmonteiro23:07:34

because we call test/delete-node-modules

anmonteiro23:07:39

and it leaves empty directories

anmonteiro23:07:49

on the subsequent npm install NPM thinks the modules are installed

anmonteiro23:07:55

when really the folders are empty ¯\(ツ)

dnolen23:07:24

commented out the test on master

dnolen23:07:41

switching gears, but happy to try a patch if you come up with one

anmonteiro23:07:10

fixed now I think, running lein test

anmonteiro23:07:18

@dnolen replaced the patch with one that makes tests pass: https://dev.clojure.org/jira/browse/CLJS-2248

anmonteiro23:07:26

just ran lein test twice and couldn’t repro anymore

anmonteiro23:07:39

sorry to have made you waste so much time on this

anmonteiro23:07:34

also updated the Travis file to run Node 6

dnolen23:07:00

no worries, it’s going to take a while to get all this Node coolness under control 🙂

anmonteiro23:07:39

also waiting apply before the next release: https://dev.clojure.org/jira/browse/CLJS-2239

anmonteiro23:07:48

docstrings for :target :nodejs under self-host

anmonteiro23:07:54

to get the string-requires goodness

dnolen23:07:49

ok pushing up priority so I don’t forget

anmonteiro23:07:22

hrm I’m seeing a regression with :foreign-libs and :target :nodejs