replace delete with assigning to undefined#1007
replace delete with assigning to undefined#1007anonrig wants to merge 1 commit intoopennextjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: e86d4c6 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: |
| // Next.js doesn't parse body if the property exists | ||
| // https://github.com/dougmoscrop/serverless-http/issues/227 | ||
| delete req.body; | ||
| req.body = undefined; |
There was a problem hiding this comment.
The comment above explicitely says "the property exists"
There was a problem hiding this comment.
Might be worth an upstream pr to next.js tho I suspect it's been tried before.
The key challenge with using delete here is that it immediately puts req into a slower / non-optimized mode for all property accesses. Whether or not that moves the needle or not in performance depends on usage pattern. In this particular case, let's leave this as delete req.body and see if we can try to fix upstream.
but as a general rule, we should be avoiding delete like the plague.
| continue; | ||
| } | ||
| const keyLower = key.toLowerCase(); | ||
| if (keyLower === "set-cookie") { |
There was a problem hiding this comment.
is that a change in behavior?
vicb
left a comment
There was a problem hiding this comment.
Let's take more time to evaluate the changes in this PR (see my inline comments).
It should not have a huge impact anyway
| // We need to remove the set-cookie header from the parsed headers because | ||
| // it does not handle multiple set-cookie headers properly | ||
| delete parsedHeaders[SET_COOKIE_HEADER]; | ||
|
|
There was a problem hiding this comment.
I see this is removed but not replaced?
There was a problem hiding this comment.
oh, I see, you're removing it in the util.ts change instead.
| if (keyLower === "set-cookie") { | ||
| // We need to remove the set-cookie header from the parsed headers because | ||
| // it does not handle multiple set-cookie headers properly | ||
| continue; |
There was a problem hiding this comment.
side note... I keep thinking that it would be nice if Object.entries(...) (and the iterable pattern in general) would accept a filter function...
Object.entries(headers, filter);...that could optimize this pattern more and make it so the collection only had to be iterated through once... That's for another time tho.
Replacing
delete X.ywithX.y = undefinedavoids killing any optimizations done by V8 to access the properties of an object faster.Subset of #1002