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.

Sign up for GitHub

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

Jump to bottom

ref(typescript): Remove DOM from default lib types #5816

Closed
wants to merge 4 commits into from
Closed

ref(typescript): Remove DOM from default lib types #5816

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ebf0ce
Remove `dom` from default lib types
timfish Sep 26, 2022
9c1af09
Actually use `url` module in node as it's not global in node v8
timfish Sep 26, 2022
bb0da37
Fix test errors
timfish Sep 26, 2022
b0b22a2
Remove erroneous comment
timfish Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/angular/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"experimentalDecorators": true
"experimentalDecorators": true,
"lib": [
"ES6",
"DOM"
]
}
}
11 changes: 7 additions & 4 deletions packages/browser/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}
6 changes: 3 additions & 3 deletions packages/core/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
}
Expand Down
16 changes: 11 additions & 5 deletions packages/core/tsconfig.test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
{
"extends": "./tsconfig.json",

"include": ["test/**/*"],

"include": [
"test/**/*"
],
"compilerOptions": {
// should include all types from `./tsconfig.json` plus types for all test frameworks used
"types": ["node", "jest"]

"types": [
"node",
"jest"
],
"lib": [
"ES6",
"DOM"
]
// other package-specific, test-specific options
}
}
1 change: 1 addition & 0 deletions packages/ember/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"noEmit": true,
"baseUrl": ".",
"module": "esnext",
"lib": ["ES6", "DOM"],
"experimentalDecorators": true,
"paths": {
"dummy/tests/*": ["tests/*"],
Expand Down
13 changes: 8 additions & 5 deletions packages/gatsby/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"jsx": "react"
"jsx": "react",
"lib": [
"ES6",
"DOM"
]
}
}
15 changes: 10 additions & 5 deletions packages/hub/tsconfig.test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
{
"extends": "./tsconfig.json",

"include": ["test/**/*"],

"include": [
"test/**/*"
],
"compilerOptions": {
// should include all types from `./tsconfig.json` plus types for all test frameworks used
"types": ["jest"]

"types": [
"jest"
],
"lib": [
"ES6",
"DOM"
]
// other package-specific, test-specific options
}
}
11 changes: 7 additions & 4 deletions packages/integrations/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"esModuleInterop": true,
"lib": [
"ES6",
"DOM"
]
}
}
11 changes: 7 additions & 4 deletions packages/nextjs/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}
2 changes: 1 addition & 1 deletion packages/node/src/requestdata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,8 @@ function extractQueryParams(req: PolymorphicRequest): string | Record<string, un

return (
req.query ||
(typeof URL !== undefined && new URL(originalUrl).search.replace('?', '')) ||
Copy link
Collaborator Author

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.

Copy link
Member

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

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?)

Copy link
Collaborator Author

@timfish timfish Sep 27, 2022 •
edited

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!

// In Node 8, `URL` isn't in the global scope, so we have to use the built-in module from Node
new url.URL(originalUrl).search.replace('?', '') ||
url.parse(originalUrl).query ||
undefined
);
Expand Down
5 changes: 3 additions & 2 deletions packages/node/test/domain.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ describe('domains', () => {
expect(hub.getStack()[1]).toEqual({ client: 'process' });
// Just in case so we don't have to worry which one finishes first
// (although it always should be d2)
// setImmediate
setTimeout(() => {
d1done = true;
if (d2done) {
done();
}
});
}, 0);
});

d2.run(() => {
Expand All @@ -66,7 +67,7 @@ describe('domains', () => {
if (d1done) {
done();
}
});
}, 0);
});
});
});
13 changes: 8 additions & 5 deletions packages/react/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"esModuleInterop": true,
"jsx": "react"
"jsx": "react",
"lib": [
"ES6",
"DOM"
]
}
}
12 changes: 8 additions & 4 deletions packages/remix/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
"jsx": "react",
"module": "es2020"
"module": "es2020",
"lib": [
"ES6",
"DOM"
]
}
}
13 changes: 9 additions & 4 deletions packages/svelte/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"compilerOptions": {}
"include": [
"src/**/*"
],
"compilerOptions": {
"lib": [
"ES6",
"DOM"
]
}
}
10 changes: 7 additions & 3 deletions packages/tracing/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}
2 changes: 1 addition & 1 deletion packages/typescript/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"importHelpers": true,
"inlineSources": true,
"isolatedModules": true,
"lib": ["es6", "dom"],
"lib": ["es6"],
"moduleResolution": "node",
"noErrorTruncation": true,
"noFallthroughCasesInSwitch": true,
Expand Down
10 changes: 7 additions & 3 deletions packages/utils/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}
10 changes: 7 additions & 3 deletions packages/vue/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}
11 changes: 7 additions & 4 deletions packages/wasm/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{
"extends": "../../tsconfig.json",

"include": ["src/**/*"],

"include": [
"src/**/*"
],
"compilerOptions": {
// package-specific options
"lib": [
"ES6",
"DOM"
]
}
}