search

nervous-systems / serverless-cljs-plugin

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

Graceful lumo exit on compiler failure. Support lumo >= 1.8.0. #22

Closed arichiardi closed 6 years ago

arichiardi commented 6 years ago

This patch fixes the fact that serverless could not display compilation ailures because lumo was not returning anything. Now lumo.build.api accepts a callback that returns the error. This feature is only available in the latest lumo (>= 1.8.0) so the README now makes clear that other versions won't be supported.

arichiardi commented 6 years ago

Yep I will change it and yes that is why I think we should wait for a merge/release. Unfortunately the old versions cannot be made to work as well. Note that lumo compilation has always been experimental so I don't think we can rely on version numbers these. The good news is that the next release will probably be at par with JVM cljs.

On September 13, 2017 1:35:24 AM PDT, Moe Aboulkheir notifications@github.com wrote:

moea commented on this pull request.

  • (run! classpath/add! inputs)
  • (lumo.build.api/build
  • (apply lumo.build.api/inputs inputs)
  • compiler-opts))
  • (js/Promise.
  • (fn [resolve reject]
  • (js/console.info
  • "Invoking the Lumo compiler w/ inputs"
  • (.stringify js/JSON (clj->js inputs)))
  • (run! classpath/add! inputs)
  • (lumo.build.api/build
  • (apply lumo.build.api/inputs inputs)
  • compiler-opts
  • nil

  • (let [promise-fn (if (:error %) reject resolve)]

  • (promise-fn %))))))

This would be clearer if the only statement inside the promise was:

(js/Promise.
 (fn [resolve reject]
   (lumo.build.api/build
      ...
      (fn [result]
        (if (:error result)
          (reject result)
          (resolve result))))

What happens if the user doesn't have the latest lumo?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/nervous-systems/serverless-cljs-plugin/pull/22#pullrequestreview-62364706

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

arichiardi commented 6 years ago

My PR is not in lumo yet and I am waiting for feedback from Antonio. I fixed this, but we'd better wait. In the meantime, I fixed another problem in another PR.

arichiardi commented 6 years ago

So this is now in lumo 1.8.0-beta and I don't think it is a feature that can be retrofitted: before the lumo.build.api call was only able to return the compilation result synchronously. This means that we versions older than this new one, if compilation fails things gets weird.

arichiardi commented 6 years ago

On second thought, this one now should work for older lumos as well because the compiler function probably throws on errors, making the promise reject anyways.

moea commented 6 years ago

Yeah, but if the signature doesn't include a callback argument, presumably it'll be a problem?

arichiardi commented 6 years ago

Oh right, well what do you think is the best approach? I think lumo compilation before this version is not worth keeping compatibility with, as you have probably guess it was very experimental.

moea commented 6 years ago

If we clearly document the status of the lumo dependency in the README (either a commit hash or version #, whatever's applicable), then I'm fine with the change.

arichiardi commented 6 years ago

Ok I will add that, there is an actual version published on npm, 1.8.0-beta1 .

moea commented 6 years ago

If you get a moment, can you address the previous review comment which suggested removing everything from the Promise monad except for the final build call?

arichiardi commented 6 years ago

Sure, do you mean the console.log? The add-classpath is better inside in case it throws

moea commented 6 years ago

If it throws outside the promise, the process will exit irregularly in any case.

arichiardi commented 6 years ago

So I think that thanks to https://github.com/anmonteiro/lumo/commit/80192d10b15d13de5439fd146daf9ca60efec124 this patch can be entirely dropped. The build api is sync now.