nervous-systems / serverless-cljs-plugin

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

WIP - Add lumo script for building the lambda zip #6

Closed arichiardi closed 7 years ago

arichiardi commented 7 years ago

Hi! :smile: here I am with the script :tada:

Path is maybe off for a script, I just wanted to put it out there.

Thanks to this I am able to compile (cljs bootstrap compatible) lambdas. I am now missing the deploy part and that is why I am going to ask some help here :smile:

I also have a question about the serverless.yml file itself, because maybe we could define custom entries for bootstrapped compilation (which requires extra care), for instance:

 handler-lambda-cljs:
    cljs-lumo: handler.core/my-lambda

Open to thoughts - suggestions - corrections!

moea commented 7 years ago

@arichiardi I appreciate this - I'm going to have to learn a little about lumo before merging - a couple of high-level things maybe you can help with:

To keep it simple, let's keep serverless.yml the same, and have the 'use-lumo' thing be a switch accepted by serverless-cljs-plugin. Either all cljs functions are lumo, or all are jvm.

arichiardi commented 7 years ago

Lumo is like the vanilla cljs compiler, so you pass things in. I will probably then need to read the project.clj file from the script.

The output of the zip can be passed to the zip! function. Now it is hardcoded to .serverless

About index.js, I thought it was a convention coming from the cljs-lambda template but you are right, somebody could choose not to use that.

moea commented 7 years ago

@arichiardi - in order:

moea commented 7 years ago

@arichiardi This is a great idea!

arichiardi commented 7 years ago

Just to give more context, this has to be executed with:

lumo serverless-cljs-lambda/js_build.cljs param1 param2

So we can pass things from the command line as well as reading project.clj directly

moea commented 7 years ago
arichiardi commented 7 years ago

Ok so basically what you'd need is to modify the build! function, it seems all the parameters you describe are already input to compile! and zip! at the moment.

One of the nice conventions that I have seen more and more around is to have edn files. So we could baptize here our cljs-lambda.edn which will contain compiler options, inputs..same as the :cljs-lambda that at the moment is nested in project.clj.

How does it sound?

Also, are you going to spawn a new branch or you want to continue on this one? Just so I don't rebase it. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

moea commented 7 years ago

That's fine if you prefer that, it ends up being more or less the same thing - as long as the file is optional, and the script uses sensible defaults both for values unspecified in the file, and when the file does not exist. Do you want to implement that bit?

I'll branch from these changes.

arichiardi commented 7 years ago

Well it is definitely easier to start from edn, then once the feature is ready we can think of parsing yaml (which is definitely more familiar for JS users, but is always limited)... I guess that yes I can hook the edn.

The plugin in the feature if necessary could in order scan:

But ok step by step. In any case cool stuff!

arichiardi commented 7 years ago

A question, should I keep the structure of the current cljs-lambda config map? I guess it would be a bit easier for your users to understand and for your to wrap your head around.

For me, I like something like:

{:project {:name "logpoc" 
           :source-paths ["src"] 
           :target-path "target"
           :resource-paths ["resources"]}
 :compiler {:options {:target        :nodejs
                      :language-in   :ecmascript5
                      :optimizations :simple
                      :main "logpoc.core"}}
 :functions [{:name "logpoc-dev-bunyan-lambda"
              :invoke logpoc.core/bunyan-lambda}]}

Where :inputs has become a project property and the naming follows the ones in boot and lein.

What do you think?

Ps.: For the generation there is either https://github.com/fotoetienne/cljstache or https://github.com/janl/mustache.js.

moea commented 7 years ago

@arichiardi I added some superficial commentary - when you're ready, if you reopen the pull request against the branch wip/lumo, I can make the index changes there, and will discuss with you before merging into master.

arichiardi commented 7 years ago

I am actually thinking about a change that will pass --functions at the comment line, new call:

lumo -c scripts -m lumo.build --zip-path .serverless/logpoc.js --functions ....

What do you think?

I see that functions is computed in index.js...so it cannot go in cljs-lambda.edn option.

moea commented 7 years ago

That's what we need, yeah - it's edn, so e.g. --functions '[{:name ...} ...]' - see index.js for specifics.

arichiardi commented 7 years ago

Ok so this last version works fine with:

lumo -c scripts -m lumo.build --zip-path .serverless/logpoc.js --functions "[{:name logpoc :invoke logpoc}]"

Quotes are necessary. I will push the changes to wip/lumo if there are not objections.

arichiardi commented 7 years ago

Ok @moea no objections but I need to be Collaborator in order to push to wip/lumo :smile:

arichiardi commented 7 years ago

Btw thanks to this the lumo command will be greatly simplified.

moea commented 7 years ago

@arichiardi I can add you as a collaborator, but can't you just open a PR against that branch and I can merge it?

arichiardi commented 7 years ago

Oh, never tried that, I thought you can only merge to master from github unless you change the default branch. Are you sure it is going to work?

moea commented 7 years ago

https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

arichiardi commented 7 years ago

Wow awesome did not know that :)

moea commented 7 years ago

@arichiardi I am pressed for time, but hope to have an example with index generation etc. working by the end of the week.

arichiardi commented 7 years ago

I will try that too then, I am pressed for time as well ;) Thanks for your hints, this is going to be a very nice feature

moea commented 7 years ago

@arichiardi Let's not both do the same thing!

arichiardi commented 7 years ago

Ok you are right let me do that then, I need to make this work soon...I was wondering if the local invoke can be worked on as well...for sure you are more familiar with it. What do you think?

moea commented 7 years ago

@arichiardi I wouldn't feel comfortable delegating the index generation task, it needs to be as similar as possible to what cljs-lambda is doing, and work for all optimization levels. If you are in a hurry to get it working, just manually write an index file once for your project, and add it to the zip.

I am personally not particularly interested in local invoke - I can just call the exposed function from a REPL. If you are interested in it, and it doesn't involve far reaching changes, then I am happy to include it.

arichiardi commented 7 years ago

Ok sounds good.

moea commented 7 years ago

@arichiardi If you create a project which depends on serverless-cljs-plugin 0.2.0-alpha, then serverless deploy --lumo or serverless package --lumo should work. I've only tested with optimizations :none. See lumo-example in the branch wip/lumo for a working example.

moea commented 7 years ago

Note that I changed the format of the config file and made it optional - see commits in the branch for details.

arichiardi commented 7 years ago

Oh you went for a cmd line option for that, I was thinking of having a lumo key instead of cljs in serverless.yml, it is more convenient (less to type and to think about, what if I forget the switch? :smile:). I am checking the commits ;)

moea commented 7 years ago

Having lumo instead of cljs would be per-handler, so it would give the mistaken impression that it's possible to mix both cljs and lumo handlers in a single serverless.yml, which it's not.

It would be possible to also support a top level custom lumo: true option, but I don't think it makes a huge difference.

arichiardi commented 7 years ago

I like the lumo: true option better - or even cljs-compiler: lumo. I can contribute that, is it going to be in opts in the plugin?

moea commented 7 years ago

Yeah, so ideally it should work if you do either this:

custom:
  lumo: true

or deploy with --lumo. I think changing the if condition to if(_.get(serverless.service, 'custom.lumo') || opts.lumo) is sufficient.