-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Web Streams cleanup #56819
Web Streams cleanup #56819
Changes from all commits
5117ed1
b383545
1648a1e
7253f6c
d930873
5b04279
c8bf05b
d0b0bdc
675d20d
b325d8f
e5e27f6
34f6286
0ebb92f
c33cda8
9b754da
c242a5f
6f3f023
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 |
---|---|---|
|
@@ -8,12 +8,11 @@ export function requestToBodyStream( | |
stream: Readable | ||
) { | ||
return new context.ReadableStream({ | ||
start(controller) { | ||
stream.on('data', (chunk) => | ||
start: async (controller) => { | ||
for await (const chunk of stream) { | ||
controller.enqueue(new KUint8Array([...new Uint8Array(chunk)])) | ||
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. Do we need to construct a 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. This was matching the existing beheviour seen in: |
||
) | ||
stream.on('end', () => controller.close()) | ||
stream.on('error', (err) => controller.error(err)) | ||
} | ||
controller.close() | ||
}, | ||
}) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ import { | |
DEFAULT_SANS_SERIF_FONT, | ||
} from '../shared/lib/constants' | ||
const capsizeFontsMetrics = require('next/dist/server/capsize-font-metrics.json') | ||
const https = require('https') | ||
|
||
const CHROME_UA = | ||
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.61 Safari/537.36' | ||
|
@@ -22,30 +21,9 @@ function isGoogleFont(url: string): boolean { | |
return url.startsWith(GOOGLE_FONT_PROVIDER) | ||
} | ||
|
||
function getFontForUA(url: string, UA: string): Promise<String> { | ||
return new Promise((resolve, reject) => { | ||
let rawData: any = '' | ||
https | ||
.get( | ||
url, | ||
{ | ||
headers: { | ||
'user-agent': UA, | ||
}, | ||
}, | ||
(res: any) => { | ||
res.on('data', (chunk: any) => { | ||
rawData += chunk | ||
}) | ||
res.on('end', () => { | ||
resolve(rawData.toString('utf8')) | ||
}) | ||
} | ||
) | ||
.on('error', (e: Error) => { | ||
reject(e) | ||
}) | ||
}) | ||
async function getFontForUA(url: string, UA: string): Promise<string> { | ||
const res = await fetch(url, { headers: { 'user-agent': UA } }) | ||
return await res.text() | ||
Comment on lines
+24
to
+26
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. This was too simple to pass up updating 😅 |
||
} | ||
|
||
export async function getFontDefinitionFromNetwork( | ||
|
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.
Nit: Why not just return a promise, and resolve that promise with the response?
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.
toResponse
is (as expected) called every request. As soon as you callawait
it will pause execution to get the value of the promise (even if it's already resolved). To avoid unnecissary pauses, this check bypasses that.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.
But this is an async function, so this will always have at least 1 tick delay. If you just return a promise resolved to the Response, we keep the same number of overall ticks and (slightly) simplify this method.