Misc improvements to lumo build #7

arichiardi · 2017-08-23T17:49:57Z

This patch adds various improvements to the lumo build, notably:

handle archiver warning
add some useful debug logging
fully forward vector as params to archiver's directory() and file()
handle custom.cljsCompiler option in serverless.yml

arichiardi · 2017-08-23T17:54:22Z

I don't know how to change it against wip/lumo...

arichiardi · 2017-08-23T17:54:54Z

serverless-cljs-plugin/serverless_lumo/build.cljs

-          :let [k (keyword (str/replace k #"^-{1,2}" ""))
-                k (cli-option-map k k)]]
-      [k (parse-option k v)])))
+        (for [[k v] (partition 2 argv)


Sorry, I will fix this change, it slipped in.

moea · 2017-08-23T18:18:39Z

index.js

@@ -52,8 +52,13 @@ function basepath(config, service, opts) {

 function cljsLambdaBuild(serverless, opts) {
  const fns = slsToCljsLambda(serverless.service.functions, opts);
+  const compiler = _.get(serverless.service, 'custom.cljs-compiler');


Hyphenated keys are going to look out of place in a serverless.yml. Please use lumo: true.

moea · 2017-08-23T18:19:06Z

index.js

+  if (process.env.SLS_DEBUG) {
+    serverless.cli.log(`Compiler is set to: ${compiler}`);
+  }
+


This is gratuitous, it will fail immediately if the compiler is incompatible with the project, so we don't need a log message.

moea · 2017-08-23T18:20:25Z

serverless-cljs-plugin/serverless_lumo/build.cljs

-       (doseq [d (zip-opts :dirs)]
-         (.directory archiver d))
-       (.file archiver (zip-opts :index) #js {:name "index.js"})
+       (doseq [d (:dirs zip-opts)]


Please remove the log messages, it makes the code noisy and is not particularly helpful here.

moea · 2017-08-23T18:21:51Z

serverless-cljs-plugin/serverless_lumo/build.cljs

-       (.file archiver (zip-opts :index) #js {:name "index.js"})
+       (doseq [d (:dirs zip-opts)]
+         (js/console.log "adding directory %s" (.stringify js/JSON (clj->js d)))
+         (if (string? d)


Please remove this conditional and always assume a directory is a string. If we need this in the future for some reason, we can add it.

moea · 2017-08-23T18:22:28Z

serverless-cljs-plugin/serverless_lumo/build.cljs


 (defn write-index [content outpath]
+  (js/console.log "writing %s..." outpath)


Please remove these log messages.

moea · 2017-08-23T18:23:06Z

serverless-cljs-plugin/serverless_lumo/build.cljs


 (defn ^:export -main [& args]
  (let [opts (cli-options *command-line-args*)
-        conf (read-conf!)]
-    (build! opts (merge-with merge default-config (read-conf!)))))
+        file-config (read-conf!)


Please also remove these log calls.

moea · 2017-08-23T18:26:26Z

serverless-cljs-plugin/serverless_lumo/build.cljs

+    (js/console.log "config is %s" (.stringify js/JSON (clj->js config) nil 2))
+    (.then (build! opts config)
+           #(js/console.log "zip created: %s" %1)
+           #(js/console.error "Error:" (.-message %1)))))


Please set the process exit code to 1 if we catch an error here.

moea · 2017-08-23T18:27:16Z

serverless-cljs-plugin/serverless_lumo/build.cljs

       (.on output-stream "close" #(resolve-fn output-path))
+       (.on archiver "warning" #(if (= (.-code %1) "ENOENT")


Please use a function with named parameters.

arichiardi · 2017-08-23T18:55:35Z

Why remove? It is so so useful when you have problems, you can ask a user to execute to send the output of the "Executing:" command..don't you think? -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

moea · 2017-08-23T19:03:28Z

@arichiardi That hasn't been my experience - it's not helpful enough to justify the clutter. The serverless plugin logs the parameters it's passing to the script - everything else that's being logged could just as well be obtained by asking for the config file, or asking for a directory listing.

arichiardi · 2017-08-23T19:23:56Z

Ok, I can do that but I feel you really close for future kinds of compilers. And then you need to push a major version for the change? What is the advantage of a boolean? -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

arichiardi · 2017-08-23T19:24:56Z

I can use camel case -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

moea · 2017-08-23T19:29:42Z

If you prefer that, use camel case.

arichiardi · 2017-08-23T19:34:48Z

I think it makes more sense. Will change it. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

arichiardi · 2017-08-23T19:36:00Z

Ok will remove the commit. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

arichiardi · 2017-08-23T22:25:44Z

Ok addressed all the change requests and added some doc to the README

moea · 2017-08-23T23:17:52Z

Thanks - why is 1.7.0+ required, just so I know?

arichiardi · 2017-08-24T06:54:53Z

Because there is was a breaking change from lumo.core/command-line-args to cljs.core.

We could actually support both namespaces in our code but 1.7.0 will be broken unless we use an unmangled get on global.

anmonteiro/lumo#237

moea · 2017-08-24T07:44:55Z

@arichiardi Why are we using *command-line-args* if the -main function receives the args as a sequence? With older versions of lumo, can you verify that receiving the arg vector still works? I would very much like to remove the version restriction.

arichiardi · 2017-08-24T07:46:26Z

The reason is node sends all the args, even the ones passed to lumo itself and we don't want those.

Sure I can tweak that.

moea · 2017-08-24T07:50:37Z

Yeah, let's work around it.

This patch allows to specify the cljs compiler of the build. At the moment only lumo is the alternative to the default JVM compiler (which requires lein), but in the future others can be added (a vanilla cljs script, shadow-cljs, boot-cljs...).

moea · 2017-08-24T10:25:49Z

@arichiardi Regarding the fix, is command-line-args empty, or unbound in older versions of lumo?

arichiardi · 2017-08-24T10:32:02Z

Premise: the above works for the three versions regardless, so seq does the right thing even when unbound.

In 1.6.0 we have only bound lumo.core, in 1.7.0 bound only cljs.core. In the bugged version, the lumo.core JS mangled object is filled but not bound in cljs, where cljs.core is but will always be nil.

arichiardi · 2017-08-24T10:33:46Z

Fix anmonteiro/lumo@fcba19a

arichiardi · 2017-08-24T10:34:45Z

Btw I have just noticed there that I don't need seq because it is already done in lumo.

moea · 2017-08-24T10:35:07Z

Presumably you can use the unmangled syntax to refer to the lumo.core var -
lumo.core/*command-line-args*?

arichiardi · 2017-08-24T10:36:14Z

@moea not that would not work in 1.7.0. That is why I used the mangled version 😉

moea · 2017-08-24T10:37:25Z

If lumo.core is required, why would it not work?

arichiardi · 2017-08-24T10:41:28Z

Good point! Maybe it is not required anymore? I have not investigated the why in depth, a bit in a hurry today.

moea · 2017-08-24T10:42:05Z

I mean if you just add (:require [lumo.core]) to the module, I don't see how it cannot work.

arichiardi · 2017-08-24T10:56:40Z

No I don't think it can work because the defonce is missing and therefore it is not in the compiler state.

However the global JS object is filled up at runtime so we can query it.

moea · 2017-08-24T11:02:30Z

If the namespace is required, you will get a compiler warning about an undefined var, and nil at runtime. Please make the change.

arichiardi · 2017-08-24T11:07:21Z

Will do, probably more verbose and uglier than what is now, but hey, you're the boss 😄

arichiardi · 2017-08-24T11:45:59Z

Awesome, it seems like it is nice and working now 😉

* Add lumo script for deploying self-host lambdas (#6) * Simplify option parsing, misc. fixes * Working end-to-end example * Misc improvements to lumo build (#7) * Handle archive warning * Use apply for sequable :files entries in zip! * Make keyword lookup idiomatic * Handle custom.cljsCompiler option in serverless.yml This patch allows to specify the cljs compiler of the build. At the moment only lumo is the alternative to the default JVM compiler (which requires lein), but in the future others can be added (a vanilla cljs script, shadow-cljs, boot-cljs...). * Add Lumo compiler section to README * Work-around lumo's command-line-args bug and support all versions * Use `simple` template for `advanced` compilation * 0.2.0

arichiardi force-pushed the lumo-improvements1 branch from 20c31da to 83e34ae Compare August 23, 2017 17:50

arichiardi changed the title ~~Misc improvements~~ Misc improvements to lumo build Aug 23, 2017

arichiardi commented Aug 23, 2017

View reviewed changes

moea changed the base branch from master to wip/lumo August 23, 2017 18:18

moea reviewed Aug 23, 2017

View reviewed changes

arichiardi force-pushed the lumo-improvements1 branch 6 times, most recently from 748cb7d to a590a65 Compare August 23, 2017 22:24

arichiardi added 5 commits August 24, 2017 02:32

Handle archive warning

f4ad454

Use apply for sequable :files entries in zip!

5b95ed2

Make keyword lookup idiomatic

e67ce27

Add Lumo compiler section to README

ca5b4da

arichiardi force-pushed the lumo-improvements1 branch from a590a65 to b01bac0 Compare August 24, 2017 09:33

arichiardi force-pushed the lumo-improvements1 branch from b01bac0 to 35e7093 Compare August 24, 2017 10:36

Work-around lumo's command-line-args bug and support all versions

2e817c3

arichiardi force-pushed the lumo-improvements1 branch from 35e7093 to 2e817c3 Compare August 24, 2017 11:07

moea merged commit 3bb0de4 into nervous-systems:wip/lumo Aug 24, 2017

arichiardi deleted the lumo-improvements1 branch August 24, 2017 11:19


		(defn write-index [content outpath]
		(js/console.log "writing %s..." outpath)

		(.on output-stream "close" #(resolve-fn output-path))
		(.on archiver "warning" #(if (= (.-code %1) "ENOENT")

Misc improvements to lumo build #7

Misc improvements to lumo build #7

Conversation

arichiardi commented Aug 23, 2017 • edited Loading

arichiardi commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moea Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi commented Aug 23, 2017 via email

moea commented Aug 23, 2017

arichiardi commented Aug 23, 2017 via email

arichiardi commented Aug 23, 2017 via email

moea commented Aug 23, 2017

arichiardi commented Aug 23, 2017 via email

arichiardi commented Aug 23, 2017 via email

arichiardi commented Aug 23, 2017

moea commented Aug 23, 2017

arichiardi commented Aug 24, 2017

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017

moea commented Aug 24, 2017

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017 • edited Loading

arichiardi commented Aug 24, 2017

arichiardi commented Aug 24, 2017

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017 • edited Loading

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017

moea commented Aug 24, 2017

arichiardi commented Aug 24, 2017

arichiardi commented Aug 24, 2017

arichiardi commented Aug 23, 2017 •

edited

Loading

moea Aug 23, 2017 •

edited

Loading

arichiardi commented Aug 24, 2017 •

edited

Loading

arichiardi commented Aug 24, 2017 •

edited

Loading