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

Avoid using res #4

Closed
nfroidure opened this issue Nov 12, 2016 · 7 comments
Closed

Avoid using res #4

nfroidure opened this issue Nov 12, 2016 · 7 comments
Assignees
Labels

Comments

@nfroidure
Copy link

nfroidure commented Nov 12, 2016

Since i emitted a lil critic of this module in a blog post i think i have to submit an issue for the problem i raise in it to be totally fair.

I think this module should not assume its users to have any res object and stay the closest possible to the problem it solves by letting users to define were they want to pick/set the vary header value:

const value = res.getHeader('Vary');

const newValue = vary(value, 'Origin')

res.setHeader('Vary', newValue);

In short, it should be more functionnal ;). I know it is a breaking change and you probably do not plan to do so but just dropping the idea.

For ref.:
https://github.com/nfroidure/blog/blob/4b6bfdccea8ad404dcc2bc116fa609f220388a5c/contents/pages/en/blog/http_rest_apis_with_nodejs.html#L346-L360
Till a draft

@dougwilson
Copy link
Contributor

dougwilson commented Nov 12, 2016

Um, that is what the vary.append function does. I'm really confused because your asking for something we already provide?

@dougwilson
Copy link
Contributor

dougwilson commented Nov 12, 2016

Your example above is done with the module today, but maybe you didn't even read my readme, which is very hurtful, especially if you are writing a blog post to bash a module saying it should have a feature it already does and even documents on the readme that is smaller than your blog post :(

const value = res.getHeader('Vary');
const newValue = vary.append(value, 'Origin')
res.setHeader('Vary', newValue);

@dougwilson dougwilson self-assigned this Nov 12, 2016
@dougwilson
Copy link
Contributor

dougwilson commented Nov 12, 2016

Closing as invalid, because this feature is already provided.

@nfroidure
Copy link
Author

You are perfectly right and i'm really sorry to have disturb you with that stinky issue. I took time to read the full readme prior to the append addition and i stayed on my first opinion. Hope you won't mind me, i'll remove that part of my blog post before publishing it.

@dougwilson
Copy link
Contributor

FWIW, I agree that the modules should not require req/res, etc. and have pure functions. They were originally created with req/res, though they remain for convenience, but would add pure functions to any that are missing some if there is interest expressed. The vary.append was added back in 2014 upon request from users :)

@nfroidure
Copy link
Author

I'll lurk more, i promise ;). It is a great thing you're open to such additions, i will PR if i see that purely functionnal features can be added in the JS HTTP projects.

@dougwilson
Copy link
Contributor

Yea. So these modules were originally built-ins to the like of Connect/Express, but those projects were chopped up to individual modules so that other people would re-use them. Of course, that means that their interfaces currently may prefer the usage of Express/Connect, but I have at least worked to ensure they can always be used with only the base Node.js objects. I personally don't have too much a need for the pure functions in my day-to-day work, so of course an not greatly adding them myself, but definitely don't mind adding any that are missing if there is a desire :) It's always tricky to create a module (or API) that you would never use, because you could have created an unusable API and you'd never know it ;) so being able to add it where there will be a live use-case somewhere is the best case scenario to make it a useful API.

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

No branches or pull requests

2 participants