-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
lib: unify ICU and no-ICU TextDecoder #61409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
367908c to
5a2317f
Compare
4d6ff8f to
b2e1ece
Compare
69e9173 to
adb2410
Compare
adb2410 to
fb0ca32
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61409 +/- ##
==========================================
- Coverage 88.52% 88.50% -0.02%
==========================================
Files 704 704
Lines 208802 208830 +28
Branches 40318 40311 -7
==========================================
- Hits 184842 184833 -9
- Misses 15947 15968 +21
- Partials 8013 8029 +16
🚀 New features to boost your workflow:
|
| let StringDecoder; | ||
| function lazyStringDecoder() { | ||
| if (StringDecoder === undefined) | ||
| ({ StringDecoder } = require('string_decoder')); | ||
| return StringDecoder; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lazy utility in the internal utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avivkeller this is not new code, it's just moved to an outer scope
Ideally all that should go away with follow-up fixes, string_decoder path is invalid anyway
I don't think it's worth refactoring it further
Better reviewed with whitespace ignored
this[kHandle]was not set in no-ICU versionI don't think this is major, and reporting the actual encoding name is better in that case.
This will allow further fixes and common fast path for UTF-16 / fixed impl for UTF-16 as string_decoder doesn't implement UTF-16 per spec.
UTF-16 is still broken in no-ICU version here, this PR does not address that.
Tracking: #61041