macchiato-framework / macchiato-core

Ring style HTTP server abstraction for Node.js
MIT License
378 stars 35 forks source link

Macchiato is broken in :advanced compilation with Shadow-cljs #44

Closed pavel-klavik closed 3 years ago

pavel-klavik commented 4 years ago

The following line is broken when compiled in :advanced mode (default for the release mode in Shadow-cljs):

(.createServer (node/require "http") http-handler)

Getting the following error:

(intermediate value)(intermediate value)(intermediate value).rd is not a function
yogthos commented 4 years ago

Is there a reason to do advanced compilation since the code is running server side?

pavel-klavik commented 4 years ago

For my small case probably no. But it is the default mode in Shadow-cljs, so there is a reason: faster execution, smaller file size, etc. As I understand, requiring http in a different way would solve this problem. Anyway, supporting Shadow-cljs out of the box sounds like a good idea.

yogthos commented 4 years ago

Yeah I agree with shadow-cljs support, would you be up for doing a PR for the fix?

pavel-klavik commented 4 years ago

I don't think I have good enough understanding of NodeJS and how other compilation works to know the solution for this.

yogthos commented 4 years ago

No worries, myself or @jdhorwitz will try take a look in the near future.

jdhorwitz commented 4 years ago

I can start to take a look at this. Thanks for reporting @pavel-klavik !

yogthos commented 4 years ago

👍

jdhorwitz commented 4 years ago

Starting back on this, work got crazy!

yogthos commented 4 years ago

No worries, life has a habit of getting in the way of open source. :)

mraveloarinjaka commented 3 years ago

Hello, There are several parts of the code that break in release mode. Will you be against adding a dependency to https://github.com/applied-science/js-interop ? I think this should work if all the interop is handled via that library. I can try to implement that if you are ok with the additional dependency.

yogthos commented 3 years ago

Sure, that sounds good to me. I can make the release if you'd like to do a PR for the update.

mraveloarinjaka commented 3 years ago

actually I was able to do without the dependency. I am sending the PR

mraveloarinjaka commented 3 years ago

https://github.com/macchiato-framework/macchiato-core/pull/47 created

yogthos commented 3 years ago

thanks, merged