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

feat(utils): use req.cookies if available instead of parsing #2985

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

Ignigena
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

For some frameworks, req.cookies is already a parsed object representing all the cookies on a request. A couple examples include any Node application deployed with Vercel, the Sails.js framework and any Express server which has the cookies middleware installed.

I've simply modified the cookie logic to use req.cookies if it exists and to skip the parsing of req.headers.cookies. For any application that doesn't have this available, the existing logic is used.

In our case, we are deploying our Node applications through Vercel. Unfortunately the dynamic require line here ends up breaking inside of a Vercel deployment because the cookies dependency is missing (see 5.26.x behaviour vs 5.23.x behaviour in Vercel)

We've notified Vercel of the issue but since the extra parsing here is unnecessary on ours and other similar deployments, I figured I would send this out to you guys! 😄

I noticed that the extractNodeRequestData method was entirely uncovered by tests. I've added tests for the specific cookie logic I introduced but also went ahead and added basic coverage for the rest of the properties that are parsed. I tried to keep the same structure and approach from your other tests but let me know if you'd like the tests rolled back to just the changes introduced here -- or would just like style changes in general.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Ignigena Ignigena requested a review from kamilogorek as a code owner October 19, 2020 23:47
@kamilogorek
Copy link
Contributor

Awesome, thanks! :)

@kamilogorek kamilogorek merged commit 83c8e45 into getsentry:master Oct 20, 2020
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.

None yet

2 participants