nervous-systems / serverless-cljs-plugin

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

Always generate index.js if not present #16

Closed arichiardi closed 6 years ago

arichiardi commented 6 years ago

Hello @moea,

I wanted to ask and propose here this change before working on it.

In order to really decouple the compilation from everything else around serverless we could make so that whenever there is no index.js file in the root of the project (so in case of a project generated by a lein template this won't happen), we trigger the generation. Probably after the cljsLambdaBuild in index.js.

This would make it easier to add a new compiler (like shadow-cljs, or vanilla script) without duplicating the index generation part.

My plan would be to use lumo from the serverless plugin like this (wishful thinking here):

`lumo -c ${path.resolve(__dirname, 'serverless-cljs-plugin')} ` +
           `-m serverless-lumo.index ` + // <-- main called here
           `--functions '${fns}'`

What do you think? Is it something worth working on?

moea commented 6 years ago

@arichiardi Hi! This is worth working on if your work with shadow-cljs indicates that it's 100% compatible with the index.js generation used by the lumo stuff - but I don't think it's a great idea to do it pre-emptively.

arichiardi commented 6 years ago

Ok good I was thinking the same, I will include it in the same PR then. I think I want to take a stab to the shadow compiler.

arichiardi commented 6 years ago

Do you use serverless-offline? It seems it needs index.js at the project root.

moea commented 6 years ago

I've never heard of it. If it only works with Javascript, it's probably not super useful.

arichiardi commented 6 years ago

Well, a completely local mock of AWS (Gateway included!) is always useful :smile:

arichiardi commented 6 years ago

I can confirm that serverless-offline works really well when index.js is in the root of the project and I was wondering whether we should move it from where it currently is generated (that is, .serverless/index.js) to project-root/index.js.

arichiardi commented 6 years ago

Actually better would be to have it in the dir that is the parent of :output-dir.

moea commented 6 years ago

I don't think so - I would be fine supporting a command in the lumo plugin & cljs-lambda which just generates the index file and writes it to a specified location, but it's easy enough to copy it from .serverless for now.

arichiardi commented 6 years ago

Uhm too bad :smile: Why not? At the end of the day the leiningen plugin does the same and index.js is always present and managed by serverless-cljs-plugin.

moea commented 6 years ago

The leiningen plugin doesn't write the zip file to the project directory, it writes it to the build directory.

Counting on an implicitly generated index.js is janky, because it will break if the user runs lein clean (the modules required by index.js will not exist), or if there's a failed build - and, because the only way to trigger its generation is by building a zip file from the entire project (instead of running a specific command, like I suggested), the user will have to pay an unreasonable penalty any time they add or remove a function from serverless.yml and want to have that change reflected in index.js.

Having a dedicated command to write index.js is superior in every way, but still arguably overkill for a single specific use case.

arichiardi commented 6 years ago

I see your point, I thought the index.js file was always there when calling $ lein new serverless-cljs. If not, then this changes things.

I like the command and indeed it was the original purpose of this issue.

arichiardi commented 6 years ago

Alternatively, if we defaulted to build everything in .serverless, we could already take advantage of the fact that the structure would be already right:

.serverless/
├── cloudformation-template-create-stack.json
├── cloudformation-template-update-stack.json
├── index.js
├── out/
└── serverless-state.json

And then this in package.json would work:

{
  "name": "example",
  "version": "0.1.0",
  "main": ".serverless/index.js",
  ...
moea commented 6 years ago

I still think a separate command to generate it makes sense, especially if that allows us to avoid requiring a package.json - does it?

arichiardi commented 6 years ago

Yes probably. For simplicity I'd shell to lumo in order to do that, with a warning if the file is already there.

Any preference on the name? sls generate-index feels sensible.

moea commented 6 years ago

That or adding an --index arg to sls package.

arichiardi commented 6 years ago

I like yours better, maybe both is a good option too, depending on what's easier

arichiardi commented 6 years ago

Definitely more tedious to add an actual command but on second thought I feel --index can be misleading. At the end of the day index.js is always packaged/deployed. It can convey the wrong meaning.

The generation is a one-off, only-if-you-need-it thing. Maybe a flag in serverless.yml is more appropriate if a command is too much work:

custom:
  cljsGenerateIndex: true

What do you think?

moea commented 6 years ago

Just call it --index-only or whatever, any kind of switch is much, much better than editing a config file - it's not once-off, it's once-every-time-you-add-or-remove-a-function

arichiardi commented 6 years ago

Good point, you are right I hadn't considered that

arichiardi commented 6 years ago

We cannot generate the index only in case of package because serverless expects the artifact. So --index will create the index in the project root plus the arttifact. A switch in the serverless.yml still makes sense because one (me) would want to avoid thinking if the index needs regeneration or not, the cljsIndex: true would makes sure I always have the index up-to-date.

arichiardi commented 6 years ago

Added with https://github.com/nervous-systems/serverless-cljs-plugin/commit/cff74947b49a40cd256478ea1894d00b7a47d143