-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changed string parsing to use the Buffer class #4
Conversation
… the problem with string length for non-ascii characters
o.pos += len + 2; | ||
buf = new Buffer(len); | ||
buf.write(o.str.substr(o.pos)); | ||
v = buf.toString(); |
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.
This should probably be buf.toString('binary');
I think we should probably just guard Buffer usage with a check and have some outside boolean variable like 'isNode'. Then we can just do if (isNode) {
buf = new Buffer(len);
buf.write(o.str.substr(o.pos));
v = buf.toString('binary');
o.pos += v.length + 2;
} else {
v = o.str.substr(o.pos, len);
o.pos += len + 2;
} Maybe we could expand this even further for browsers that support the likes of ArrayBuffer and such? |
Added a simple guard for Buffer using the same check as Underscore.js uses to test for node.js.
function PHParse(str, encoding = 'utf8') { ArrayBuffer might be a good idea, we should definitely something since it now crashes on non-ascii characters in a browser. |
Hrmmm, I wonder if iconv-lite would help here.... it works in the browser also. |
Added two more commits, the first one fixes a weird issue with using Buffer on Ubuntu. If the Buffer has a zero length it crashes when you try to write to it, but it works fine under OSX though. I am currently using this on one of our production servers and it has been stable the last week. The second commit added some tests and jslint which I used to debug the errors. iconv-lite might be a better solution but I have not had the time to look at it. Not really sure if you want to include this since it only works under Node.js and I changed some stuff to pass the linting we use but this works for our needs. |
I changed the string parsing to use the Buffer class in node.js. This will of course make groan dependent on node.js so you might not want to include this or find another solution that aslo works in browsers.