Implement PPR support in cache interceptor #1057
Conversation
🦋 Changeset detectedLatest commit: a2e94ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| "@opennextjs/aws": minor | ||
| --- | ||
|
|
||
| Make PPR work with the cache interceptor |
There was a problem hiding this comment.
It would be nice to make it clear that it "Only works with the Adapter API" ?
There was a problem hiding this comment.
however I am not sure to understand from the code why/where the logic is scoped to the adapter handler, could you expand on that?
There was a problem hiding this comment.
It's on the Next side, basically the resume endpoint is only available using the adapter (or in minimal mode or with some patch)
| "generateResult called with unsupported cache value type, only 'app' and 'page' are supported", | ||
| ); | ||
| } | ||
| // close(); |
| if (isInternalResult(cacheInterceptionResult)) { | ||
| applyMiddlewareHeaders(cacheInterceptionResult, headers); | ||
| return cacheInterceptionResult; | ||
| } else if (isPartialResult(cacheInterceptionResult)) { |
There was a problem hiding this comment.
It's an else if, there is a third case that none of these 2 are supposed to catch (when it is an InternalEvent)
| return eventOrResult; | ||
| const cacheInterceptionResult = await cacheInterceptor(eventOrResult); | ||
| if (isInternalResult(cacheInterceptionResult)) { | ||
| applyMiddlewareHeaders(cacheInterceptionResult, headers); |
There was a problem hiding this comment.
q: would it make sense for applyMiddlewareHeaders() to return the first param?
There was a problem hiding this comment.
Yeah, I guess. I'll have to check everywhere where it's used, but yeah
| } else if (cachedValue.type === "page") { | ||
| isDataRequest = Boolean(event.query.__nextDataReq); | ||
| body = isDataRequest ? JSON.stringify(cachedValue.json) : cachedValue.html; | ||
| if (isDataRequest) { |
There was a problem hiding this comment.
if we want to change the ternary to an if then it would make sense to fold l 232 here as well?
Enhance the cache interceptor to support PPR by introducing new types and modifying the request handling logic. This allows for immediate response delivery while processing requests in the background. Additionally, improve logging for better debugging.
One thing that doesn't work right now, is with an external middleware, but it might make sense to do it later. Especially if we plan to make the StreamCreator mandatory. Right now it would require to do it at the converter level.
Only works with the Adapter API