nanoexpress / legacy

[Deprecated] Nano-framework for Node.js. Use PRO version
https://nanoexpress.js.org
Apache License 2.0
107 stars 13 forks source link

Options method does not return any data #87

Closed literakl closed 4 years ago

literakl commented 4 years ago

Preflight options stopped working. When I invoke options method from postman (or chrome) I receive no data. I use cors per route feature as described here: https://nanoexpress.js.org/testing/known-bugs#cors-per-route-bug. I have created an cors origin function and I can see that it is being called.

I tried to remove all dependencies and create the most simple reproducible project: https://github.com/literakl/nano-options. There are two identical servers: nanoexpress and express. When I submit options method from Postman while express variant is running, I receive a response. Then I switch to nanoexpress variant and I receive no response at all.

Can you please tell me what is going on? I went through my changes and the code is identical and it used to work. The same versions are used as well. Is this bug? I lost one day trying to figure out the root cause with no luck. And it is strange that express works in the same scenerio. Thank you

dalisoft commented 4 years ago

Hi. Do you need really response? Browser anyway does not use response from options. Cors will be rejected from unallowed domains. My idea was that, just reject requests like CORS idea

dalisoft commented 4 years ago

Try example from https://github.com/nanoexpress/nanoexpress/blob/master/examples/cors.js. This not implements response for Options, but it’s works for browser

literakl commented 4 years ago

Neither this implementation returns any response for options from Postman. Have you tried it? Nanoexpress does not handle the Options method. It does not return a status code, headers, nothing. It seems that the handler is ignored.

image image

literakl commented 4 years ago

Hmm, I just tried curl instead of Postman and it seems to receive a response from your example. Probably some Postman bug. I will test my code with curl as well. Thanks for patience

literakl commented 4 years ago

Your example cors.js is working with curl, while code sample at https://nanoexpress.js.org/testing/known-bugs#cors-per-route-bug does not work with curl.

This is OK with curl:

app.options('/my-route', corsPerRoute);

This does not work with curl:

app.options('/my-route', corsPerRoute, () => {});

Postman cannot handle any of these variants, probably because of missing body. It is interesting how the same code works differently with nanoexpress/express.

Anyway, can you please update https://nanoexpress.js.org/testing/known-bugs#cors-per-route-bug? And please put a note / link to middlewares as well: https://nanoexpress.js.org/middlewares

Thank you

dalisoft commented 4 years ago

Seems this issue was resolved, yes? I updated documentation related to last middleware at options route

dalisoft commented 4 years ago

About links to middlewares, it differs on each version, so users can find yourself

literakl commented 4 years ago

Would you please add a note there as well? https://nanoexpress.js.org/middlewares CORS is neccesary these days and the issue with per route is hidden under testing. Thank you

dalisoft commented 4 years ago

CORS is neccesary these days This is user option, library is provided as is, i sure almost 95% developers know this is necessary (even my Product manager know this :D)

issue with per route is hidden under testing I added reference to this issue, i think it's enough

Thank you to you for testing and reporting issue.

Good luck, have a nice day! Regards, dalisoft.