Webstreams #70
Webstreams #70
Conversation
|
|
||
| const reader = transformedStream.getReader(); | ||
|
|
||
| for (;;) { |
There was a problem hiding this comment.
| for (;;) { | |
| while (true) { |
kanske en gnutta mer lättläst?
There was a problem hiding this comment.
Praxis verkar vara att använda for…of loopar för att tugga igenom web streams – docs/api/webstreams.html#async-iteration. Varför gör vi inte det?
for await (const chunk of transformedStream) {
if (chunk.done) break;
body += chunk.value;
}| response.statusCode = statusCode; | ||
| if (location) { | ||
| response.headers = response.headers || {}; | ||
| response.headers.location = location; | ||
| } | ||
| this.destroy(); |
There was a problem hiding this comment.
Hur funkar det här nu istället, chunk.done ?
| if (text) { | ||
| return this.evaluator.ontext(text, next); | ||
| return await this.evaluator.ontext(text, controller); |
| context.breakHit = context.breakHit || context.shouldWrite(); | ||
| return context.breakHit ? next(null, { name: "esi:break" }) : next(); | ||
| if (context.breakHit) controller.enqueue({ name: "esi:break" }); |
There was a problem hiding this comment.
Varför används controller.enqueue istället för return här?
Känns som att det blandas ganska friskt med att använda controller eller return… ESIEvaluator borde väl kunna låta alla ESITag jobba med bara returvärden / kasta fel?
lib/ListenerContext.js
Outdated
| // if this is localhost and we have a computeAtEdge config when we need to map to a | ||
| // backend | ||
| if (host === this.options.localhost && self.options.computeAtEdge) { | ||
| backend = self.options.computeAtEdge.defaultBackend; | ||
| // maybe we have different backends, let's check | ||
| if (self.options.computeAtEdge.pathToBackend) { | ||
| // FIXME: implement | ||
| } | ||
| } |
There was a problem hiding this comment.
😵💫 Mycket options. Vet inte om jag hänger inte med… Är det typ i test: adress är http://localhost; i edge: adress är https://app-env.bn.nr ?
| @@ -15,19 +15,13 @@ | |||
| ], | |||
| "author": "Bonnier News", | |||
| "license": "MIT", | |||
| "peerDependencies": { | |||
| "@bonniernews/atlas-html-stream": ">=2", | |||
There was a problem hiding this comment.
Varför är det här inte längre en peer dependency? Det är ju ett paket som importas i koden. Går ju inte bara att ha som dev dependency.
| mock.method(global, "fetch", (url, options) => { | ||
| if (!(url === "http://mystuff/" && options.method === "GET" && !options.headers["content-type"])) { |
There was a problem hiding this comment.
Sånt här är ju inte helt optimalt. URL-encoding, case insensitivity, timeouts etc… Finns det någon typ polyfill av fetch man kan skicka som tredje argument så nock kan fånga upp riktiga anrop?
test/localEsiTest.js
Outdated
| if (!(url === "http://mystuff/" && options.method === "GET" && !options.headers["content-type"])) { | ||
| return { status: 404 }; | ||
| } | ||
| return { body: ReadableStream.from(evalResponse), status: 200 }; |
There was a problem hiding this comment.
Returnvärdet ska vara en promise
test/localEsiTest.js
Outdated
| if (url === "http://mystuff/case-insensitive" && options.method === "GET" && options.headers["extra-header"] === "Case insensitive") { | ||
| return { body: ReadableStream.from("header name is case insensitive"), status: 200 }; | ||
| } |
There was a problem hiding this comment.
xD testar väl inte längre case sensitivity?
| const markup = "<esi:include src=\"/mystuff/\" dca=\"none\"/><p>efter</p>"; | ||
|
|
||
| mock.method(global, "fetch", (url, options) => { | ||
| if (!(url === "http://localhost:1234/mystuff/" && options.method === "GET" && options.backend === "origin" && options.headers.foo === "bar")) { |
There was a problem hiding this comment.
fetch tar väl inte options.backend? Eller är det en egenhet i Fastly's edge miljö?
|
@markusn, is this still relevant, or can we close it? |
based on the fastly branch