-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
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
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 89.87% 89.84% -0.03%
==========================================
Files 671 671
Lines 203178 203124 -54
Branches 39062 39040 -22
==========================================
- Hits 182599 182503 -96
- Misses 12926 12972 +46
+ Partials 7653 7649 -4
🚀 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
fb0ca32 to
1204484
Compare
1204484 to
e74c6fe
Compare
|
Rebased, no actual changes |
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