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

[arichiardi]

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.

[moea]

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?

[arichiardi]

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 ***@***.***> 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: ```clojure (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: #22 (review)

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

[arichiardi]

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]

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]

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]

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

[arichiardi]

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]

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]

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

[moea]

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]

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

[moea]

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

[arichiardi]

Done, let me know if you would like me to change the style of this.

[arichiardi]

This is not needed anymore given that it has been solved in Lumo 1.8.0-beta1

[arichiardi]

I put 2 in order to differentiate it from the normal exit code for throws, which is 1.

[arichiardi]

So I think that thanks to anmonteiro/lumo@80192d1 this patch can be entirely dropped. The build api is sync now.