Remove pre-parsed headers shortcut from StreamedPHPResponse#3362
Remove pre-parsed headers shortcut from StreamedPHPResponse#3362
Conversation
…sponse() fromPHPResponse() was pre-populating parsedHeaders as an optimization to skip re-parsing headers it already had. This created a coupling between the factory method and the internal caching mechanism that made the class harder to reason about. Now fromPHPResponse() just encodes headers into the stream like before, and getParsedHeaders() lazily parses on first access like it does for every other code path.
There was a problem hiding this comment.
Pull request overview
Removes the fast-path that pre-seeded StreamedPHPResponse’s parsed-headers cache from fromPHPResponse(), so headers are always parsed lazily via the stream round-trip.
Changes:
- Renamed the internal parsed-headers cache field to
cachedParsedHeaders. - Removed the factory shortcut that pre-populated the parsed-headers cache.
- Updated
getParsedHeaders()to use the renamed cache field.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new StreamedPHPResponse( | ||
| headersStream, | ||
| stdout, | ||
| stderr, | ||
| Promise.resolve(response.exitCode) | ||
| ); |
There was a problem hiding this comment.
This change removes the previous fast-path where fromPHPResponse() could return already-parsed headers without JSON/string stream parsing. If this factory is used in hot paths (or headers are accessed frequently), this adds avoidable overhead (stream read + JSON.parse) on first access. To keep decoupling while avoiding the extra work, consider adding an optional constructor parameter (or a private initializer method) to seed the cache via a supported interface (e.g., initialParsedHeaders?: { headers; httpStatusCode }) rather than writing to the internal field from the factory.
| private cachedParsedHeaders: Promise<{ | ||
| headers: Record<string, string[]>; | ||
| httpStatusCode: number; | ||
| }> | null = null; |
There was a problem hiding this comment.
cachedParsedHeaders stores a Promise<...> rather than the parsed headers object itself, which can be misleading when reading/maintaining the code. Consider renaming to reflect that it’s a promise (e.g., cachedParsedHeadersPromise or parsedHeadersPromise) to make the caching behavior and type clearer.
| this.cachedParsedHeaders = parseHeadersStream(this.#headersStream); | ||
| } | ||
| return await this.parsedHeaders; | ||
| return await this.cachedParsedHeaders; |
There was a problem hiding this comment.
Inside an async function, return await is redundant here and slightly increases overhead by introducing an extra await step; returning the promise directly is sufficient unless you specifically need to catch/transform errors within this function.
| return await this.cachedParsedHeaders; | |
| return this.cachedParsedHeaders; |
Summary
StreamedPHPResponse.fromPHPResponse()was pre-populating the internalparsedHeaderscache as a shortcut to avoid re-parsing headers it already had in hand. This created unnecessary coupling between the factory method and the class's internal caching mechanism, making the class harder to reason about and modify (it was a source of bugs in #3361).Now
fromPHPResponse()simply encodes headers into the stream and returns.getParsedHeaders()lazily parses on first access, the same code path used everywhere else.Test plan
npx nx test php-wasm-universal— one pre-existing failure unrelated to this change)