Skip to content

Use loop for acceptParams - #6066

Merged
jonchurch merged 1 commit into
expressjs:masterfrom
blakeembrey:be/simplify-params
Nov 14, 2024
Merged

Use loop for acceptParams#6066
jonchurch merged 1 commit into
expressjs:masterfrom
blakeembrey:be/simplify-params

Conversation

@blakeembrey

@blakeembreyblakeembrey commented Oct 17, 2024

Copy link
Copy Markdown
Member

Using a loop here would improve performance for parsing over splitting into an array and iterating over it, then splitting some more. If we prefer the overall split on ; for readability though, we can still avoid the second split by using indexOf instead. There's also some slices and trim that could be avoided for extra performance but it seemed a little overkill for this function.

Finally, given the code is only used here:

express/lib/response.js

Lines 575 to 584 in a46cfdc

if(key){
this.set('Content-Type',normalizeType(key).value);
obj[key](req,this,next);
}elseif(obj.default){
obj.default(req,this,next)
}else{
next(createError(406,{
types: normalizeTypes(keys).map(function(o){returno.value})
}))
}
. If we avoid exporting these utils to users we can just return value only and avoid the rest of the processing entirely.

closeshttps://github.com/expressjs/security-triage/issues/24

@UlisesGasconUlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far seems much faster!

basic-example (2)

Report from https://jsbm.dev/ZbkL2XxwbkIOI

@UlisesGascon

Copy link
Copy Markdown
Member

Should we also backport it to v4?

@jonchurch
jonchurch merged commit 805ef52 into expressjs:masterNov 14, 2024
jonchurch pushed a commit to jonchurch/express that referenced this pull request Nov 14, 2024
jonchurch pushed a commit to jonchurch/express that referenced this pull request Nov 14, 2024
nigrosimone added a commit to nigrosimone/ultimate-express that referenced this pull request Nov 16, 2024
@blakeembrey
blakeembrey deleted the be/simplify-params branch November 21, 2024 19:52
@bjohansebasbjohansebas mentioned this pull request Mar 15, 2025
@UlisesGasconUlisesGascon mentioned this pull request Mar 23, 2025
68 tasks
@wesleytoddwesleytodd mentioned this pull request Mar 28, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@blakeembrey@UlisesGascon@jonchurch@bjohansebas