-
Notifications
You must be signed in to change notification settings - Fork 8
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
remove undici
#223
remove undici
#223
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for-await...of | ||
export async function * streamAsyncIterable (stream) { | ||
const reader = stream.getReader() | ||
try { | ||
while (true) { | ||
const { done, value } = await reader.read() | ||
if (done) return | ||
yield value | ||
} | ||
} finally { | ||
reader.releaseLock() | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
"allowJs": true, | ||
"checkJs": true, | ||
"skipLibCheck": true, | ||
"lib": ["es2021"], | ||
"lib": ["es2021", "DOM"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a better lib target to add? See https://www.typescriptlang.org/tsconfig#lib. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh man, this is a rabbit hole. See the discussion in DefinitelyTyped/DefinitelyTyped#60924. First of all, Station Core is currently supporting Node.js Then, ideally, all we need to do is add However, that does not work because of the reasons described in DefinitelyTyped/DefinitelyTyped#60924. Also, we should not be using DOM types, because Fetch API on server has slightly different API than in the browser. (This was mentioned in that DefinitelyTyped discussion too.) I quite like the workaround described DefinitelyTyped/DefinitelyTyped#60924 (comment). The downside is that we still need to have undici in our dependencies (and probably keep it up to date via Dependabot?), but at least it becomes a dev-time only dependency. 🤷🏻♂️
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is a diff of the changes I think we should make on our way to fix this properly. Unfortunately, it's not enough. diff --git a/package.json b/package.json
index edc5522..abe92f3 100644
--- a/package.json
+++ b/package.json
@@ -11,7 +11,7 @@
"station": "./bin/station.js"
},
"engines": {
- "node": ">=16"
+ "node": ">=18"
},
"scripts": {
"build": "tsc -p .",
@@ -32,6 +32,7 @@
]
},
"devDependencies": {
+ "@types/node": "^18.18.1",
"@types/yargs": "^17.0.24",
"get-stream": "^8.0.1",
"mocha": "^10.2.0",
diff --git a/tsconfig.json b/tsconfig.json
index 5d96c50..83db0fb 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -3,7 +3,7 @@
"allowJs": true,
"checkJs": true,
"skipLibCheck": true,
- "lib": ["es2021", "DOM"],
+ "lib": ["es2021"],
"target": "es2021",
"module": "Node16",
"moduleResolution": "node16", There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for doing the research! I've applied the fixes you suggested. I don't think overall this improves our situation unfortunately, and I'd rather propose to continue using undici for now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💯 agree |
||
"target": "es2021", | ||
"module": "Node16", | ||
"moduleResolution": "node16", | ||
|
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.
I feel like I'm doing something wrong here. However, MDN suggests this approach. I didn't find anything in node's stdlib for this 🤔