Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cors and options support #29

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

neimanpinchas
Copy link

I had a need for cors, so I tried to add options and cors

please forgive the coding style, I know it might not be ready for commit, I created the idea as a starting point, I tested it with my code (with haxe remoting, this is another story) and it worked.

add cors middleware before listen
rename hidden cors field to public field so user can opt in

update map of allowed methods on every get, post..
on setup generate options response string to avoid runtime cost
implicitly add options option
this was the main intent of this PR
now I think we might instead add the options handler
as a std route, instead of making a special case
Need to think about a proper typedef for cors config
with options for headers, methods, origins etc.
Placing it in listen has no effect because MW is already flattened
for the meantime as hashlink still has some bugs
remove query string from path
it is BTW removed from basepath
@neimanpinchas
Copy link
Author

I've added basic nodejs support, so no need for express js anymore even when targetting js

@PXshadow
Copy link
Owner

Well done on the changes! Could you make it so that the tests run on both hashlink and nodejs?

@neimanpinchas
Copy link
Author

OK, will do

probably only need to remove these
threading
syncronous Sys.sleep

also needs this for Http.requestUrl to work
HaxeFoundation/hxnodejs#187, hope it will get merged one day.

@PXshadow
Copy link
Owner

Latest status?

@neimanpinchas
Copy link
Author

It is not trivial for me to adjust the existing testing for javascript.

I've implemented a virtual local threading, but now I bumped into the syncronous Http.requestUrl, being used in the tests.

I think maybe we need to combine mutiple test case into a generic one, running off a json file for the input urls and the server settings

@PXshadow
Copy link
Owner

Sorry about the conflict, it seems it's only one part, the other 2 conflicting areas is an import, and a comment.

If I could ask the current status?

@neimanpinchas
Copy link
Author

The import can be removed as it is redundent anyway (I placed it into a if #hl because I want to use it with js backed)

I think that the making the Stream variable a closure instead of server class property was handled in both branches, the only diffrence is that I've used var, and he used final.

The done varible has been added for js to function properly, once the buffer exceeds the content length, however this may cause issues with persitent connections.

I wasn't yet succesfull with translating the test cases for javascript, as JS is single threaded and t is not trivial to make a synchronous web request when the same process should act as the server and respond, I am thinking something about a config file based testing, but I wasn't yet succesfull, I hope the branch will not get rusty in the meantime.

@PXshadow
Copy link
Owner

The import can be removed as it is redundent anyway (I placed it into a if #hl because I want to use it with js backed)

I think that the making the Stream variable a closure instead of server class property was handled in both branches, the only diffrence is that I've used var, and he used final.

The done varible has been added for js to function properly, once the buffer exceeds the content length, however this may cause issues with persitent connections.

I wasn't yet succesfull with translating the test cases for javascript, as JS is single threaded and t is not trivial to make a synchronous web request when the same process should act as the server and respond, I am thinking something about a config file based testing, but I wasn't yet succesfull, I hope the branch will not get rusty in the meantime.

Worst case scenario the rust can be shaken off, or this pr can be redone on latest commit. I'd for sure like to see these changes get added, so let me know how I can help make them happen.

I'd also say forget about js if that'd be helpful and focus on options and cors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants