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 checks and enhance getTokenFromHeader function definition #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Anass-Daoudi
Copy link

Hi,

In this PR, I mainly add more checks to the function getTokenFromHeader and enhance its definition structure.

Thanks for the review!
Happy to get feedbacks :)

@Anass-Daoudi
Copy link
Author

Hi @kentcdodds @anishkny,
Could you please do some little review to my PR :).
Thanks

Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, though I don't personally feel like these changes make the code easier to follow...

@Anass-Daoudi
Copy link
Author

Thanks @kentcdodds for giving a look.
But I think, the code I propose is more safe. Isn't it?

@kentcdodds
Copy link
Contributor

Technically yes, but I doubt that there's a situation where req.headers wouldn't be an object in this code path...

@Anass-Daoudi
Copy link
Author

As I see, as this code is not statically typed, we can't confirm that this function will always receive req.headers as an object; as an example, an express middleware that sets by accident req.headers=to-some-primitive-value.

Or may be this sort of controls must be done in unit tests?

Correct me please.
Thanks @kentcdodds :)

@kentcdodds
Copy link
Contributor

I personally don't feel like the risk is worth the extra effort. But I don't have any stake in this project, so do what you want.

@Anass-Daoudi
Copy link
Author

Thanks @kentcdodds for being that reactive :)

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