Use ProtoDef skipChecks flag to allow full NBT array size support

[bdkopen]

NBT arrays are allowed a length up to 32 bits, so there needs to be a way to skip checks to allow full NBT support. Adding an optional object allows this to happen.

https://minecraft.wiki/w/NBT_format#Binary_format

Based on this required PR: ProtoDef-io/node-protodef#154

Addresses: ProtoDef-io/node-protodef#150 and #85

[extremeheat]

They may be allowed to be 32bit length, but doing so will take up very large amounts of memory and it's usually indicative of invalid NBT data, or reading with the wrong encoding (little/big endain/etc). So I think this may make more sense behind a flag. Otherwise there is really no need to have the checks in protodef in the first place.

[bdkopen]

@extremeheat do you have a recommended approach on this then?

If I add a flag, it looks like I will need to add it to many of the functions that are exported here (parse, writeUncompressed, etc). I wanted to avoid adding a lot of arguments to functions if I could.

We could also consider some sort of global setting toggle, but I am not sure I like that pattern.

[extremeheat]

Yes, you would need to propagate options to all the methods. Global is not good.

[bdkopen]

Yes, you would need to propagate options to all the methods. Global is not good.

👍 I will work on that over the next few days and update here when it's complete and tested. I agree that a global setting would be worse.

[bdkopen]

@extremeheat this is ready for re-review. Passes all the tests and it worked when I included it as the package for my project.

[bdkopen]

@extremeheat Hey! What's the status with the review? Do you need anything from me?

[extremeheat]

This seems pretty ugly. Perhaps a better way would be to pass the arguments down along with .createPacketBuffer() or .parsePacketBuffer() as opposed to creating these "instances" for every option permutation.

[extremeheat]

This might be possible to implement taking advantage of ProtoDef variables introduced in ProtoDef-io/node-protodef#125

if ((arraySize > safeSize) && !ctx.noArraySizeCheck) throw Error(...)

with usage like https://github.com/ProtoDef-io/node-protodef/blob/master/examples/variable.js#L17

[extremeheat]

I don't know if the global state approach is good. What you can probably do instead is add to parseUncompressed and parseAs a skipChecks parameter, and then set the variable based on that parameter.

Eg

-function parseUncompressed (data, proto = 'big') {
+function parseUncompressed (data, proto = 'big', noArraySizeCheck) {
  if (proto === true) proto = 'little'
+ protos[proto].setVariable('noArraySizeCheck', noArraySizeCheck)
  return protos[proto].parsePacketBuffer('nbt', data, data.startOffset).data
}

[bdkopen]

@extremeheat won't your proposal still set noArraySizeCheck globally? setVariable is setting the variable value in proto which will remain set after a parseUncompressed is made.

What are your thoughts on something like this?

 function parseUncompressed (data, proto = 'big', noArraySizeCheck) {
  if (proto === true) proto = 'little'
   protos[proto].setVariable('noArraySizeCheck', noArraySizeCheck)
   const data = protos[proto].parsePacketBuffer('nbt', data, data.startOffset).data
   // Remove the previously set variable so it doesn't persist globally for `proto`
   protos[proto].setVariable('noArraySizeCheck', undefined)
   return data;
}

[extremeheat]

As long as the protos object is not accessed outside the contained prismarine-nbt context then no there is no issue. With a global state setting function you have no idea what the options are during a call to parseUncompressed, including whether another function inadvertently updated the state without your code knowing. But yes that code works also.

[bdkopen]

I typed this with a _ variable because we still need to support the deprecated callback parameter. Eventually when the deprecated functions are removed in a major version we could remove it from the typing.

[extremeheat]

I don't know why you made the PR so much more complex. The old code was fine and setting the variable to undefined is OK in JavaScript as it's a falsy condition. There's no need to update the parse() method really, the callback mechanism should probably be removed but it's a breaking change.

[bdkopen]

@extremeheat Is there a contributors guide that outlines coding practices you expect in Prismarine repos?

If you would prefer to rely on JavaScript's falsy behavior, we can go with your original suggestion. I was immediately unsetting the variable because it was making a permanent change to the declared proto and it's unclear what affects that could have on other functions in the library.

Is the plan to fully remove parse in a future major version? If so, I agree there's no point in updating it here. I removed this change.

I kept it in parseAs since I don't see any notes of that function being deprecated and I find the gzip handling there helpful.

[extremeheat]

There isn't a guideline, but we can rely on the expected behaviors of JavaScript for the most part. I think the more important thing is making sure the code is as readable as possible and in general having less code and less unnecessary checks helps a lot there.

The parse() function logic is pretty complex currently to support callbacks and promises so adding more arguments to it will make it even more complex. I'd just leave that as is.

As you added an options object please add that to the API doc

[bdkopen]

I updated the API doc to include details about the new options object.

[extremeheat]

Lint is failing

[bdkopen]

@extremeheat lint fixed. Before merging, note we will need to merge and release #154 so we can use the update package here.

[rom1504]

Missing a test, would be great to add one.

Merging

[rom1504]

/makerelease