This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-07-09
Channels
- # aleph (4)
- # beginners (31)
- # boot (33)
- # cider (7)
- # cljs-dev (263)
- # cljsrn (1)
- # clojure (33)
- # clojure-austin (2)
- # clojure-dev (1)
- # clojure-russia (6)
- # clojure-spec (19)
- # clojure-uk (7)
- # clojurescript (79)
- # cursive (21)
- # datascript (1)
- # datomic (13)
- # dirac (12)
- # emacs (15)
- # hoplon (26)
- # lein-figwheel (3)
- # leiningen (1)
- # luminus (5)
- # lumo (20)
- # mount (1)
- # off-topic (17)
- # om (13)
- # onyx (24)
- # parinfer (70)
- # pedestal (2)
- # re-frame (19)
- # reagent (1)
- # ring-swagger (3)
- # unrepl (8)
- # untangled (58)
- # yada (2)
In the context of cljs-oss
had success setting up Planck and triggering it to build with a specific version of ClojureScript. Here is a sketch:
1. I changed Planck's project.clj
to have a dep that can be driven by an environment variable: [org.clojure/clojurescript ~(or (System/getenv "CLOJURESCRIPT_VERSION") "1.9.660")]
2. I added a minimal "`.travis.yml`-only" repo at https://github.com/cljs-oss/planck which clones and builds Planck and runs its tests
3. For the sake of running it in Travis, I forked this repo to my own GitHub account, but this step is just an optional pragmatic bit for now
4. I installed the travis
client, logged in using a GitHub personal access token created for this purpose, and then had it give me a Travis API token
5. I chose which ClojureScript version I wanted to trigger the Planck build with: body='{"request": {"branch":"master", "config": {"env": {"CLOJURESCRIPT_VERSION": "1.9.671"}}}}'
6. I triggered the build: curl -s -X POST -H "Content-Type: application/json" -H "Accept: application/json" -H "Travis-API-Version: 3" -H "Authorization: token XXtokenhereXXXX" -d "$body"
7. Observed the environment variable being set in the Travis web UI log: export CLOJURESCRIPT_VERSION=1.9.671
8. Confirmed that it was actually using this to build Planck by seeing WARNING: Use of undeclared Var cljs.js/lib at line 450 out/cljs/js.cljs
which is in that version of ClojureScript
Gist of the above in case it flys into the Slack ether: https://gist.github.com/mfikes/8be162fd730774e11990255345ee1127
Here is a subsequent successful Travis build, where I triggered it to build using ClojureScript 1.9.660 via the REST API: https://travis-ci.org/mfikes/planck-1/builds/251614094
Perhaps an expedient alternative to having individual cljs-oss
projects then post the results somewhere, would be for us to instead have a main page that hosts a bunch of those Travis build status images, one per OSS project; these things: https://api.travis-ci.org/cljs-oss/planck.svg?branch=master
For the purposes of testing this in the actual cljs-oss
org, I went into GitHub and granted access to cljs-oss
to the Travis CI application
Triggered Planck in there and now you can see it https://travis-ci.org/cljs-oss
Hmm, I'm having problems getting react-dom/server
work with cljs master
Uncaught Error: Undefined nameToPath for module$home$juho$Source$reagent$node_modules$react_dom$server
server.js file exists and is converted to Closure module
cljs_deps.js refers to this as reagent.dom.server dependency:
goog.addDependency("../reagent/dom/server.js", ['reagent.dom.server'], ['reagent.impl.util', 'reagent.interop', 'reagent.ratom', 'module$home$juho$Source$reagent$node_modules$react_dom$server', 'cljs.core', 'reagent.impl.template']);
but cljs_deps.js doesn't contain line that would provide react-dom/server
Made another pass over the aget
post. I think it is in good shape now. https://github.com/mfikes/clojurescript-site/blob/30d7072c870f87804a25e05192ddd6401fc62918/content/news/2017-07-xx-proper-array-and-object-access-patterns.adoc
@juhoteperi hrm at least when running António’s tests on master I see that exact line
(let [{:keys [inputs opts]} {:inputs (str (io/file “src” “test” “cljs_build”)) :opts {:main ‘npm-deps-test.string-requires :output-dir “out” :optimizations :none :npm-deps {:react “15.6.1” :react-dom “15.6.1"} :closure-warnings {:check-types :off :non-standard-jsdoc :off}}}] (build/build (build/inputs (io/file inputs “npm_deps_test/string_requires.cljs”)) opts))
@juhoteperi run this at the REPL on master, and check out
and you’ll see what I mean
@dnolen Yes, works there... Hmm, the line is present on clean build in Reagent, but not after recompile.
Running the snippet again works, perhaps this is something caused by lein/figwheel or something
also I reverted the changes we made to the Node.js bootstrap in January - just not worth the hassle
@dnolen The aget
PR is currently updated (I squashed onto it). So, it could be merged in and then de-nitted, or whatever approach makes sense.
Oh. OK. I don't think I've seen code that switches when in :advanced
. That makes sense though.
@mfikes it’s pretty easy to do https://github.com/clojure/clojurescript/commit/0c0b3a5f32581b1084b19231844486631fd0c4c9#diff-a2be3628916249ce5ea41f4c28989172R59
@mfikes it will also be interesting to see what the perf impact will be - I wouldn’t be surprised if the checks are negligible under JSC if we always did them even at runtime
:advanced
could, over time, take on ClojureScript specific meaning of "elide expensive runtime checks" (reminds me of :elide-asserts
)
btw. having two versions of (macro) code depending on dev/advanced build setting could make sense in other scenarios as well, I mean in dev mode the emitted code could be more verbose with some useful intermediate state captured in well-named locals - this way when paused on a breakpoint user could see more useful stack frames - but this is really just a pie-in-the-sky idea
Tangentially related: If we ever have the std lib spec'd an interesting situation occurs for fns that also have macros. (The macros take over and defeat the ability to spec the fn when used in operator position.)
@mfikes, not familiar with that, but in general, in dev-mode code should be generated for developer happiness, in advanced mode for runtime/performance 🙂
but I understand that this could be very tricky, because the behaviour could unintentionally diverge and performance profile could be way off between those modes
In an alternative world (where code size isn't important), simple code that gets JITd aggressively in the target environment is nice too. Perhaps in a decade or two, everything will be clean and self-hosted. Feels like we are still in an era similar to when developers tried to keep their assembly compact.
@dnolen Re react-dom/server problem, try (def state (cljs.analyzer.api/empty-state))
and passing state as third param to build. This way I can reproduce the problem.
https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L722 analyzer uses this to check if missing-js-modules
should be added to namespace map of the ns which has require to e.g. react-dom/server
And during the second build doesn't happen as module is already present in the compiler state
@mfikes let’s get back to cljs-oss
, I looked at your work, it looks good, I would still propose not to create new .travis.yml files, but reuse the maintained ones in libraries themselves, also to your note “where to store results”, I would propose to have a special branch on main cljs-oss mono-repo which would collect all the results in commit messages (or in generated files markdown inside that branch)
do you want me to go ahead and prepare proposal for that skeletal mono-repo which would do the work? I can spend some time on it right now @juhoteperi @mfikes
@darwin Yeah, sure. My work with cljs-oss/planck was just to explore. If there are refinements we can make to simplify, that makes sense.
@mfikes it also gives your post a lot more power - since you can immediately leverage the points made
Interesting. I rebaselined it, and was wondering if the rebaseline was even needed. Will try to repro what you are seeing.
From: Mike Fikes <[email protected]> Date: Sat, 8 Jul 2017 15:56:10 -0400
I think this is because it is keeping my original "author" date when I re-baseline. I'm making a fresh clone of master and checking if I can apply.
When I apply it I get:
/Users/mfikes/Downloads/CLJS-2192-v2.patch:112: trailing whitespace.
and another similer warning@juhoteperi I’m curious why that js-module-exists?
is present at all when computing missing-js-modules
in the analyzer
@juhoteperi can you try removing it on your end and see if it fixes the issue?
If I vi
the patch file I see ^M
characters at the end of the line where it is patching the script/test.ps1
file.
@dnolen Yep, I can repro that with git am
but get the whitespace warnings with git apply
. Digging. I'll try to create a patch using exactly the instructions we have on our site.
@dnolen Doesn't work. The first time missing-js-modules
gets value #{react-dom/server}
, next time #{module$home$juho$Source$clojurescript$node-modules$react-dom$server}
@juhoteperi hrm how does that happen?
Looks like the require values in analyzer are already resolved to the module name
@dnolen I am able to apply the patch by using --ignore-whitespace
. I think the reason is that it patches a mixture of Unix and DOS files. Here is a transcript showing a fresh clone, etc.
$ ls
CLJS-2192-v2.patch
$ git clone
Cloning into 'clojurescript'...
remote: Counting objects: 33824, done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 33824 (delta 7), reused 23 (delta 6), pack-reused 33790
Receiving objects: 100% (33824/33824), 12.34 MiB | 11.39 MiB/s, done.
Resolving deltas: 100% (16156/16156), done.
$ cd clojurescript/
$ git am ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities
error: patch failed: script/test.ps1:7
error: script/test.ps1: patch does not apply
Patch failed at 0001 CLJS-2192: Add ChakraCore testing facilities
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
$ git am --abort
$ git am --ignore-whitespace ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities
OK @dnolen, empirically it appears the --keep-cr
option works. It results in the same warning that git apply
warns about, but results in the patch being applied, and the DOS file (`script/test.ps1`) having the DOS line endings being properly handled in the patched result.
$ git am --keep-cr ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities
.git/rebase-apply/patch:107: trailing whitespace.
@{ env="NASHORN_HOME"; name="Nashorn"; cmd={ & "$env:NASHORN_HOME\jjs" $testjs } },
.git/rebase-apply/patch:108: trailing whitespace.
@{ env="CHAKRACORE_HOME"; name="ChakraCore"; cmd={ & "$env:CHAKRACORE_HOME\ch" $testjs } }
warning: 2 lines add whitespace errors.
It looks like the option was added to git to handle funky Mail line endings. But it works for this mixed DOS/Unix patches. FWIW: https://stackoverflow.com/a/24178810/4284484
This site PR accompanies the ChakraCore patch: https://github.com/clojure/clojurescript-site/pull/109
it could possibly be a nice reference point for other functional languages targeting JS
@juhoteperi I think I found the fix
we just need to dissoc :js-module-index
when collecting sources so that build becomes idempotent again
So problem with tests or problem with the npm-deps feature?
@dnolen That would fix this. I don't know enough about analyzer side to know if that will break something else.
@anmonteiro npm-deps feature
@juhoteperi pretty sure it cannot - this is only in the sources collecting pass
Oh, I know the bug
We call -find-sources
twice
And the 2nd time we've already processed modules
So there's no point missing-js-modules
being there
I don't think that causes the problem with cljsbuild/figwheel/etc
I don't know if we do
It was already like that and I didnt wanna touch it
Dissocing from what?
Ah, yeah
I think we remove the ones that we know about in parse-ns
Perhaps that's the bug
I'm on phone so just taking guesses from memory
@anmonteiro anyhow I have a quick fix that I don’t think can cause problems
Awesome, thanks
I feel like CLJS test coverage went way up too for this release
Maybe we could also highlight that in one of the posts
just created this repo with some notes and docker wrapping working @mfikes, @juhoteperi: https://github.com/cljs-oss/canary
Also hoping to get CLJS-1428 in, that'll be huge for bridging yet another gap between Lumo and Planck
the tool still needs implementation, but it should be pretty easy: https://github.com/cljs-oss/canary/blob/master/runner/src/cljs_oss/runner.clj#L27
@dnolen maybe this is adding unnecessary churn, but hasn't the Node.js bootstrapping changed over time in Closure? https://github.com/google/closure-library/blob/master/closure/goog/bootstrap/nodejs.js
Are we up to date with that or can we learn something from it?
@anmonteiro the only difference is the sync load thing
which is kind of interesting, for :modules
support under Node.js I guess - though that seems strange to me
@juhoteperi try master when you can
@anmonteiro landed CLJS-1428
Exciting
@dnolen Still having problems: No such namespace: module$home$juho$Source$reagent$node-modules$react-dom$server .....
@juhoteperi but your cljs_deps.js
looks OK?
But I think it won't write that as that exception is thrown
Exception is thrown before Cljs compiler writes the cljs_deps.js file
@juhoteperi we have a test that builds twice - so I need some clarity as to what you’re observing
I have an idea, testing
make sure you don’t have anything weird going on here like a target
directory if you’re testing in the repo
Cool, CI can eliminate the need to bisect 🙂 https://github.com/mfikes/clojurescript/commits/master Logged: https://dev.clojure.org/jira/browse/CLJS-2201
fixing
I could swear that was passing for me?
@mfikes that’s probably a bogus failure
assumes you set the output-dir to whatever it’s set when running script/test
Maybe I understand now: missing-js-modules are collected after -find-sources
is called, that will only collect the sources from build
source
files/dirs etc. Not files from classpath.
@juhoteperi should we be doing more?
Except in Reagent src
is in cljsbuild source-paths
so it should be included in -find-sources
... Not sure yet.
^ oh, self-parity tests
@juhoteperi @anmonteiro oh yeah - that’s going to be a problem
that means :npm-deps
won’t work correctly for packages libs (JARs) with sources w/ string requires
And projects where Cljs inputs is a single file and other files are loaded from classpath.
attached patch to https://dev.clojure.org/jira/browse/CLJS-2201
^ not sure how this is going to play with chakracore
oh, nevermind. we’re not running tests on windows
problem will be the add-implicit-options
, but probably the foreign-libs part from that can also be moved after add-dependency-sources
yeah, the problem is adding the implicit options
I agree we should be able to do that
@juhoteperi nice catch!
Testing this now
I can work on this, for now I have problems and I'm unsure on how to reproduce those other than with Reagent
happy to do it as well, my only problem is I wouldn’t know how to write a test
@anmonteiro just put a dep on the classpath that you require
and that dep has a string require
trying that now. @juhoteperi or do you wanna fix this?
@anmonteiro I'm already working on this
you go for it!
hrm I think this is a bug in the REPL:
cljs.user=> (require 'clojure.string :reload-all)
#{"foo.core" "goog.dom.NodeType" "cljs.spec.gen.alpha" "goog.array" "clojure.walk" "goog.reflect" "goog.asserts" "goog.object" "clojure.string" "goog.math.Long" "cljs.repl" "goog.string" "goog.debug.Error" "cljs.spec.alpha" "goog.math.Integer" "cljs.core" "cljs.pprint" "goog.string.StringBuffer"}
shouldn’t we be returning nil
?
Already patch attached for this @anmonteiro in one of the recent JIRAs. Digging it up.
thanks!
I think I now have string requires in classpath files working, and now I can reproduce the same problems as with Reagent. If file that uses string requires is modified -> recompile doesn't find the JS file. No files modified -> cljs_deps.js is missing the line for JS file.
Aand works with Reagent
blah, noticed a typo in one of the comments
no... it is correct 😄
@dnolen figured out the proper patch for CLJS-1764: https://dev.clojure.org/jira/browse/CLJS-1764
my current understanding is that we need to bind ana/*analyze-deps*
to false
because we’re computing & loading them manually
does this assumption break anything?
I don’t think it breaks anything
What is best way to provide process.env.NODE_ENV
currently?
I have process.env
ns with goog-define
but looks like React code is being loaded before that ns
@juhoteperi had to provide a shim ns process/env.cljs
in the past
@anmonteiro I think that may be correct re 1764
@juhoteperi if it’s just for testing try putting 'process.env
in preloads
@dnolen yeah I’m piloting the REPL with cljs-1764
locally and it doesn’t seem to break anything
Okay, with preloads process.env works
@anmonteiro yeah I think it’s ok, add-dependencies puts everything in dep order
It turns out we are passing array-like JavaScript objects to aget
(the primary example being arguments).
@juhoteperi the tests you added, are they broken without the fix?
did some more extensive testing of the REPL patch (CLJS-1764) locally in a largeish project
couldn’t find any edge cases
@anmonteiro Yes, the were broken with the added classpath file
I’m asking because I thought it wouldn’t be in the classpath
like, I don’t think src/test/cljs_build
is ever added to the classpath, and we specify only 1 input https://github.com/clojure/clojurescript/blob/master/src/test/clojure/cljs/build_api_tests.clj#L241
just trying to prevent a regression
cf. directories in the classpath: https://github.com/clojure/clojurescript/blob/master/project.clj#L8-L10
src/test/cljs
is in classpath
The build input file is under src/test/cljs_build
, but new file is under src/test/cljs/
missed that, thanks
it was there all along for me to see… string_requires_in_classpath.cljs
sorry for the noise
@anmonteiro Dunno if you are interested in https://dev.clojure.org/jira/browse/CLJS-2204 (it may have been behind the other failure)
looking
hrm I guess the fix to that is to just include src/test/cljs_build
in the lein classpath for testing
I don’t think there’s any problem in doing that
trying
@mfikes added a patch to CLJS-2204, can you make sure it works for you?
both lein test
and script/test
pass for me
Hmmmm, I don't think is correct solution. That folder is supposed to be outside of classpath.
lein test
should work without changes
lein test
works
the problem is script/test
because it tries to compile that npm-deps thing
maybe the right solution is to create yet another directory under src/test
that lein knows about but script/test
doesn’t
Oh right, because it tries to complie any files under src/test/cljs
it was not a good idea to put that file there
New directory is a good solution
I’ll modify my patch to do that
revised the patch to include a new cljs_cp
directory
maybe we could even migrate some things under src/test/cljs
that are not used by script/test
to this folder
pushing it even further, remove src/test/cljs
from the lein test classpath
if there are no common things between script/test
and lein test