-
-
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(typescript): Remove DOM
from default lib
types
#5816
ref(typescript): Remove DOM
from default lib
types
#5816
Conversation
It looks like this actually picked up a bug in |
This isn't quite as simple as I'd hoped as there are issues around the tests and |
Can we change |
We can, but first we'll need a common global type and there might be some issues surrounding the use of |
@@ -310,8 +310,8 @@ function extractQueryParams(req: PolymorphicRequest): string | Record<string, un | |||
|
|||
return ( | |||
req.query || | |||
(typeof URL !== undefined && new URL(originalUrl).search.replace('?', '')) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On node v8 URL
would have always been undefined
whereas now it'll parse the url correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On node v8
URL
would have always beenundefined
Yup - that's what the url.parse()
line is there for.
The reason there's the bare URL
is because for a hot second this code lived in utils and therefore needed to be compatible with the browser as well. Given that if any part of it ever ends up back in the land of shared code it'll be this part, I'm tempted to say we should just leave this be. (Unless is straight-up broken, but I don't believe that's the case, is it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - that's what the url.parse() line is there for.
🤦🏻♂️
Looks like a version of this code still lives in utils with an InjectedNodeDeps
:
sentry-javascript/packages/utils/src/requestdata.ts
Lines 339 to 342 in 12c19d0
function extractQueryParams( | |
req: PolymorphicRequest, | |
deps?: InjectedNodeDeps, | |
): string | Record<string, unknown> | undefined { |
I'm tempted to say we should just leave this be
Given that we're not supporting node v8 for much longer this change is almost certainly gratuitous but after removing dom types, TypeScript was throwing a build error!
The errors were in the actual test code files so fixing was only a matter of adding |
Another option is to leave the |
I listed that in the PR description as a possible alternative and looking at this PR now it feels compelling. It just felt like dom types really should be opt-in rather than the default. 🤔 |
The base TypeScript config currently includes DOM types:
sentry-javascript/packages/typescript/tsconfig.json
Line 10 in a5bfa80
In the spirit of supporting other JavaScript runtimes (#5611), we need to keep these types out of the core packages.
This PR removes
DOM
from the base tsconfig lib types and adds it back to all the packages where it's required. For now this effectively removes the DOM types from@sentry/types
,@sentry/core
,@sentry/hub
and@sentry/node
.@sentry/utils
will follow later when all the browser code has been moved!This PR also formats the
tsconfig.json
files.Pros
Cons
@sentry/electron
,@sentry/react-native
,sentry-cordova
and@sentry/capacitor
may need to add"lib": ["ES6", "DOM"]
to theirtsconfig.json
.Alternatives
Override with
"lib": ["ES6"]
in the packages where there shouldn't be any DOM types.