replace delete with assigning to undefined #1007
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?
| // 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.
3 participants
Replacing
delete X.ywithX.y = undefinedavoids killing any optimizations done by V8 to access the properties of an object faster.Subset of #1002