agrueneberg / Corser

CORS middleware for Node.js
MIT License
91 stars 11 forks source link

[QUESTION] [SUGGESTION] #20

Closed jessevdp closed 7 years ago

jessevdp commented 7 years ago

Hey, I was reading through the docs for your npm module and got kinda stuck on the part where you explain the configuration Object.

I am working on a server that's supposed to be configured through a config file. I also want to make the origins I allow configurable. (Not just allow all of them by just calling corser.create() as middleware). However I got stuck on the type of the origins property. Also, can you insert an * into the property of the config object to allow all origins anyway? (Else I have to create some code to make sure everything works the way I want it too)

Suggestion

You might want to pimp your readme file a bit to make it more clear for people on how to use your module. I personally document all my code using JSDoc and then use a handy tool to write that JSdoc into markdown format. You can see the result of this in the documentation of my npm module called magister-tools.

The handy module I use to do this can be found is called jsdoc-to-markdown

If you want to do it your way it's fine

However, please make sure to include the type of every property on the config Object and a clear description of what it does. Along with any special values you can put into the property etc. (like a * or something)

I recommend putting it into a markdown table like this for readability but that's totally up to you!

property-name type Description
Example string An example string

It would be cool if you updated your README file but just an answer would do fine!

I realize not everyone has as much time as I do etc.

Awesome module BTW!

agrueneberg commented 7 years ago

Hi Jesse, thanks for the feedback! I will look into improving the README as soon as possible after working through my Thanksgiving pile. To answer your question, if you want to allow all origins (*), you simply omit the origins property from the options object. I think I understand your confusion: most properties of the options object act like whitelists, but the default behavior is "allow all".

jessevdp commented 7 years ago

So simply leaving out the origins property of the options object? If I understand correctly?

Alright, that'll be a small workaround but no real problem!

jessevdp commented 7 years ago

Or wait,

If I load my configuration file and the field where I specify the whitelist is empty? Does setting the origins whitelist to an empty array allow all origins?

Since I will set it.. But to smth empty...

agrueneberg commented 7 years ago

Yes, an empty array will have the same effect as omitting the field: all origins will be allowed. I will make some changes to the description of the origins field, "unbound" is coming straight from the CORS spec and actually means something different in JavaScript.

agrueneberg commented 7 years ago

I have changed the wording of the origins property in 6493f5096327262ff598c918542a15abd931236f. Please let me know if it is easier to understand now.

I have also considered your suggestion to use tables instead of paragraphs to describe the properties of the configuration object, but decided against it because maintaining larger tables in GitHub Flavored Markdown manually seems too much of a hassle. I may give JSDoc another chance in the future, though, so thanks for the recommendation.

jessevdp commented 7 years ago

Awesome! Yes that does help!

As for the tables, yeah I wouldn't manually create these tables either. Markdown is just too messy.