Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Check whether or not Buffer and Int32Array are present and support buf[index] #23

Closed
wants to merge 1 commit into from

5 participants

@GraemeF

As I mentioned in creationix/jsonparse#10 I've run into a problem with a combination of browserify/zombie where browserify's implementation of Buffer can't implement the [] operator which jsonparse makes use of. I guess the same will happen for other polyfills too.

This pull request intends to check that whatever is given to jsonparse supports [], falling back to Int32Array and then Array if Buffer is either not available or won't work with jsonparse. The tests now pass using buffer-browserify's Buffer where they previously failed.

As you can probably tell I'm no JS expert - particularly when it comes to client-side stuff - so am prepared for ridicule/schooling as appropriate :grin:

@dominictarr
Owner

So, it still looks to me that this is an issue with zombie, did you post an issue on zombie?
@substack what do you think? maybe this patch should go into buffer-browserify?

@GraemeF

I didn't because I'm able to reproduce it with the JSONStream tests just by using buffer-browserify and, although I don't fully understand why Int32Array is there (so I'm not supremely confident that that bit is right), it's clear to me that the use of buffer[] at https://github.com/creationix/jsonparse/blob/master/jsonparse.js#L91 isn't gonna work with the "writing and reading via buffer[number] does not work" limitation described in toots/buffer-browserify#1.

I also tested with an Int32Array polyfill and had the same problem: these things are unable to implement [] due to JavaScript not allowing them to.

@GraemeF

And yeah it's a weird one to find the right place for a fix because so many things are involved... Ideally @creationix (and everyone else) would be able to avoid using things in Buffer that aren't (and can't be) supported outside of node, but I dunno if that's possible.

I put it here because this is where the thing is created, and you were already checking to see if Buffer is there or not.

@dominictarr
Owner

it's just that the way I see it, the browsers are already right, because they are just browsers.
zombie only exists to make testing browser code easier, so, if there is some way that it is not like a browser,
then that is a bug in zombie.

Maybe you can make a tiny test case (just pulling enough out of buffer-browserify to reproduce your test

assert.strictEqual(new Buffer('X'), 88)

And whatever is the minimum from buffer-browserify to get this test failing in zombie, but passing in a browser.
At least you should post an issue on zombie and link back to this one.

@GraemeF

I'm working on that now, while trying not to pull in the whole universe with the tiny test case. :wink:

Do you see what I mean about it not working in browsers if anyone is using browserify and does a require('buffer') in one of their source files or dependencies? It's just not possible to fix it in buffer-browserify due to a language restriction.

Unfortunately when you have things that can only partially emulate other things it all gets a bit messy where those things are used, if you want them to work. :confused:

@maxogden
Collaborator

just ran into this same problem. this pull req should use int8 arrays and should be on this repo https://github.com/toots/buffer-browserify/

@maxogden
Collaborator

actually haha i already fixed this 7 months ago #9

but for some reason it isnt working anymore.... i'm looking into it

@maxogden
Collaborator

ok the reason it doesnt work is the pull req i made predates buffer-browserify and now doesnt work because the Buffer from buffer-browserify doesnt use typed arrays like I did in #9. so a pull req to buffer-browserify that does essentially what #9 does would fix things OR another workaround is to use the typed array XHR response type which would bypass all the buffer crap and work with jsonparse today

@GraemeF

Ah cool, thanks @maxogden. I'll admit I was getting a bit out of my depth :worried:

Would a fix in buffer-browserify work? The JSONStream code would still be using the [] operator which (as I understand it) can't be implemented on anything that isn't really an array or Buffer, which would include the Buffer from buffer-browserify.

@dominictarr
Owner

fixing buffer-browserify to use typed arrays seems like the best way forward.

@dominictarr
Owner

is this still a problem?

@substack

This is still a problem, yes. It was made worse in browserify 2.6.0, which detects for global Buffer references and includes a Buffer definition as necessary so detecting for the presence of Buffer will not work anymore. I started on a patch for buffer-browserify to make indexed access work by extending the array prototype but haven't gotten it working yet. The place to fix this issue in the cleanest way will be in buffer-browserify.

@GraemeF

That would be great! If buffer-browserify can be made to support [] then we can all relax and forget this ever happened... :relieved:

@GraemeF GraemeF referenced this pull request in toots/buffer-browserify
Open

node.js simple tests #1

@toots

Hi guys! I'd be very happy to fix this in buffer-browserify and would welcome any patch for that. However, typed array don't seem to be supported in enough browser to be a suitable solution..

@GraemeF GraemeF referenced this pull request in pgte/duplex-emitter
Closed

breaks in 0.10.0 #4

@substack

You can close this issue since Buffer in browserify supports indexing correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 4 deletions.
  1. +23 −4 index.js
View
27 index.js
@@ -10,6 +10,25 @@ var Parser = require('jsonparse')
*/
+/*
+
+ In order to use jsonparse we need something that supports buf[index]
+ so check what works in the current environment.
+
+*/
+
+var isSafeToUseBuffer =
+ ('undefined' !== typeof Buffer) &&
+ (new Buffer('X')[0] === 88)
+
+var isSafeToUseInt32Array =
+ ('undefined' !== typeof Int32Array) &&
+ function(){
+ var buf = new Int32Array(1)
+ buf[0] = 42
+ return buf[0] === 42
+ }()
+
exports.parse = function (path) {
var stream = new Stream()
@@ -72,12 +91,12 @@ exports.parse = function (path) {
stream.writable = true
stream.write = function (chunk) {
if('string' === typeof chunk) {
- if ('undefined' === typeof Buffer) {
+ if (isSafeToUseBuffer) {
+ chunk = new Buffer(chunk)
+ } else {
var buf = new Array(chunk.length)
for (var i = 0; i < chunk.length; i++) buf[i] = chunk.charCodeAt(i)
- chunk = new Int32Array(buf)
- } else {
- chunk = new Buffer(chunk)
+ chunk = isSafeToUseInt32Array ? new Int32Array(buf) : buf
}
}
parser.write(chunk)
Something went wrong with that request. Please try again.