fix: StringT: re-support other string encodings #62
Conversation
|
I'm still investigating what is happening here. |
src/String.js
Outdated
| @@ -95,9 +95,12 @@ function encodingWidth(encoding) { | |||
| switch(encoding) { | |||
| case 'ascii': | |||
| case 'utf8': // utf8 is a byte-based encoding for zero-term string | |||
| case 'x-mac-roman': | |||
There was a problem hiding this comment.
fontkit uses a few others as well. Need fallback handling? Or is it an API usage issue?
There was a problem hiding this comment.
As this issue can cause an infinite loop, I would expect either:
- a fallback
- an explicit exception if the value is not handled and will possibly cause an infinite loop
There was a problem hiding this comment.
I don't think there's an infinite loop here. Perhaps the calling code or its callers have a loop?
There's an explicit exception on line 109
There was a problem hiding this comment.
@renchap for now, I have it assume 1-byte, which matches previous behavior
- add a test for 'x-mac-roman' - add two utf-16 alias names - if an encoding is otherwise unknown, assume 1-byte length (this matches prior behavior) Fixes: foliojs#60
srl295
changed the title
srl295
changed the title
|
This seems to fix the fontkit issue. The ideal would be a reproduction for the react-pdf issue, but i think is good to go |
There was a problem hiding this comment.
As this issue can cause an infinite loop, I would expect either:
- a fallback
- an explicit exception if the value is not handled and will possibly cause an infinite loop
I did a careful analysis of the while loop. I could not find any possibility of an infinite loop. It was, in my understanding, the exception in encodingLength() that was causing the hang, my guess because it was being handled in another layer and causing the stream reader to never emit end-of-stream -- which I think could be considered a separate bug in that layer. However I have not yet had time to analyse at that level. I will have a go when I am online again (travelling all this week with limited online availability).
The changes look good to me, thank you @srl295 for jumping on this while I was incommunicado. Apologies to all for the regression.
| //TODO: assume all other encodings are 1-byters | ||
| //throw new Error('Unknown encoding ' + encoding); | ||
| return 1; |
There was a problem hiding this comment.
It would be nice if the consumer could be warned of the unknown encoding, but not sure that there is facility for that.
Note that the pattern of throwing on an unknown encoding is a pattern copied from existing code in this same module, specifically in StringT.byteLength():
Lines 150 to 151 in fcf7d64
So is there a latent issue in byteLength() that may bite future users? I would assume that it does not need to be addressed as urgently as this.
There was a problem hiding this comment.
^ the throw may have only been on encode() or bytelength() before, which is a more restricted set of encodings.
There was a problem hiding this comment.
x-mac-roman is actually a known encoding, as well, it's handled by the underlying encoder. So one thing that could be done is to expand the set of encodings here to be exhaustive.
There was a problem hiding this comment.
AFAIK this PR brings back the previous behavior which is good enough. Lets do the minimal changes, unless there's a reproduction with an actual bug
|
@blikblum thanks. and I assume this will get a patch release also? By the way 3.0.1 is absent from https://github.com/foliojs/restructure/releases |
|
I have write access to github but don't have npm publish permission. @devongovett can publish a new version |
|
Thank you for fixing! Looking forward to a release. |
|
@devongovett friendly ping, would it be possible to get a new release, eg. This is causing some confusing breakage (indefinite hanging with custom fonts, but only some of them) in |
|
Done! |
Fixes Unsupported string encodings #60
Fixes Newest version of restructure doesn't support all string encodings fontkit#331 - allows fontkit's unit tests to pass
Probably fixes Version 3.0.1 causes
react-pdfrenderToStreamto never return #61add a test for 'x-mac-roman'
add two utf-16 alias names
if an encoding is otherwise unknown, assume 1-byte length
(this matches prior behavior)
History: #59 enabled support for 2-byte utf encodings, which were previously broken. So #59+#62 presents no regression for any other encodings.