-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ref(node): Move request-data-extraction functions to@sentry/utils
#5257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lobsterkatie
force-pushed
the
kmclb-move-requestdata-fns-to-utils
branch
from
June 13, 2022 18:52
a58d622
to
e8774f2
Compare
size-limit report 📦
|
lobsterkatie
force-pushed
the
kmclb-move-requestdata-fns-to-utils
branch
2 times, most recently
from
June 13, 2022 19:01
239ee22
to
ee1316e
Compare
AbhiPrasad
reviewed
Jun 14, 2022
lobsterkatie
force-pushed
the
kmclb-move-requestdata-fns-to-utils
branch
2 times, most recently
from
June 15, 2022 03:17
a6249e0
to
230c07d
Compare
lobsterkatie
force-pushed
the
kmclb-move-requestdata-fns-to-utils
branch
from
June 15, 2022 03:49
230c07d
to
eecf768
Compare
AbhiPrasad
approved these changes
Jun 15, 2022
AbhiPrasad
pushed a commit
that referenced
this pull request
Jun 22, 2022
#5287) This fixes an issue introduced by #5257, which was designed to be backwards-compatible, but which missed a key use case: When users pass options to our Express middleware, they might be (and in fact, for now, almost certainly are) passing them in the form of `ParseRequestOptions`, not `AddRequestDataToEventOptions`. This allows for that possibility, as a backwards-compatibility measure until v8. It also adjusts a spot in the serverless SDK where a similar problem can occur. Fixes #5282.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #5190 for a detailed description of the motivation for and main substance of this change. TL;DR, isomorphic frameworks like Nextjs need this code to live in a neutral location (rather than in the node package) so it can be used in a browser setting as well.
This is a second take on #5190, which had to be reverted because it relied on a peer dependency (
cookie
) which browser-only apps wouldn't have installed. Even if those code didn't usecookie
, they still failed to compile because withoutcookie
installed,@sentry/utils
didn't typecheck correctly.This fixes that problem by using
cookie
(andurl
, for node 8 compatibility) only as injected dependencies, not as direct ones. It also creates node-specific versions of the relevant functions (parseRequest
, now renamedaddRequestDataToEvent
, andextractRequestData
) which do the injection automatically. If the dependencies aren't passed (as would be the case in a browser setting, or when using the functions directly from@sentry/utils
), the code about cookies no-ops, and the code about URLs usesURL
, which should be defined in all modern browsers and Node 10 and above.Other changes from the original PR:
handlers.ts
, while the legacy code itself lives in its own file. This both makes it easier to delete in the future and ensures that treeshaking algorithms which only work file-by-file (rather than function-by-function) are able to exclude that code. (Though it's being kept until v8 because it's part of the node package's public API, it's no longer used anywhere in the SDK.)CrossPlatformRequest
type for use in place ofExpressRequest
. A few helper functions have also been renamed to make them less Express-centric.Ref: WEB-957