refactor: move getCharset helper to only usage#707
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors charset extraction by moving the getCharset helper from the shared lib/utils.js module into lib/read.js, where it is exclusively used, and updating the implementation to use optional chaining to avoid unnecessary string work when no charset is present.
Changes:
- Remove
getCharset(and itscontent-typedependency) fromlib/utils.jsexports. - Add a local
getCharsetimplementation tolib/read.jsand inline thecontent-typedependency there. - Switch charset normalization to
charset?.toLowerCase()to avoid allocating''and lowercasing when charset is absent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/utils.js | Removes getCharset helper and the content-type import from the shared utils export surface. |
| lib/read.js | Adds content-type dependency and reintroduces getCharset locally for request charset parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bjohansebas
left a comment
There was a problem hiding this comment.
Is this still relevant, @Phillip9587? It doesn't seem like it is anymore.
5fc041a to
791c801
Compare
|
It's not that relevant anymore as we upgraded to |
...and uses optional chaining. In theory it short-circuits faster and by that avoids the empty string allocation and
.toLowerCase()call. But this is only in theory and probably highly optimized in engines.Same ideas as @Ayoub-Mabrouk in #557.