nervous-systems / serverless-cljs-plugin

Serverless plugin for Clojurescript deployment w/ cljs-lambda
The Unlicense
75 stars 10 forks source link

Misc improvements to lumo build #7

Closed arichiardi closed 7 years ago

arichiardi commented 7 years ago

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

arichiardi commented 7 years ago

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

arichiardi commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

moea commented 7 years ago

If you prefer that, use camel case.

arichiardi commented 7 years ago

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

arichiardi commented 7 years ago

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

arichiardi commented 7 years ago

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

moea commented 7 years ago

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

arichiardi commented 7 years ago

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.

https://github.com/anmonteiro/lumo/pull/237

moea commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

Yeah, let's work around it.

moea commented 7 years ago

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

arichiardi commented 7 years ago

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 commented 7 years ago

Fix https://github.com/anmonteiro/lumo/pull/237/commits/fcba19a61203f9a5f3483195a39f0ecac6feadfb

arichiardi commented 7 years ago

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

moea commented 7 years ago

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

arichiardi commented 7 years ago

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

moea commented 7 years ago

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

arichiardi commented 7 years ago

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

moea commented 7 years ago

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

arichiardi commented 7 years ago

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 commented 7 years ago

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

arichiardi commented 7 years ago

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

arichiardi commented 7 years ago

Awesome, it seems like it is nice and working now :wink: