Offroaders123 / NBTify

A library to read and write NBT files on the web!
http://npm.im/nbtify
MIT License
42 stars 5 forks source link

NBTData Content Default Unknown #32

Closed Offroaders123 closed 1 year ago

Offroaders123 commented 1 year ago

Looked into setting the default of the NBTData<infer ThisOne> generic to unknown rather than any, but I don't know how to do that as to satisfy the generic's constraint T extends RootTag, since unknown doesn't satisfy it.

The ideal goal:

export class NBTData<T extends RootTag = unknown, const U extends FormatOptions = FormatOptions> { }

That way you don't get any as the default type for using a plain NBTData declaration. If you did want to unsafely traverse through the data without knowing it's shape, then you could pass in NBTData<any> explicitly. This would help prevent accidental usage of NBTData<any> when it wasn't intended as the result on purpose. It would allow you to catch instances where you aren't using types on the data's shape, resolving it by adding your own types for the data, or by manually casting it to any if you didn't have explicit types for it. Leaving it with the default T extends RootTag = any removes the catch step, so you yourself have to track if you are using the data through any, or by the shape you passed into the generic. It won't error for using any by default.

Ok, while writing that out, maybe the solution is to make the first generic non-optional! Then it would force you to specify a type, be it either an interface for the shape of the data, or any if you wanted to do it unsafely! I'll try that out.

Offroaders123 commented 1 year ago

Further discussion/realizations from a thread in the Minecraft Manipulator Discord, about the final line in the comment above:

If the generic was optional, the catch for the implicit nature of the generic is when you use it, it will be unknown But making the generic non-optional means you have to decide ahead of time whether it's strongly-typed, or turtle-brained as any So it moves the first step to the beginning, when the variable is defined, not when it's accessed So it removes unknown from it altogether Optional Generic state: either unknown, interface ThisIsMyShape, or any Non-Optional Generic state: ERROR: ts(missing generic parameter), interface ThisIsMyShape, or any

Offroaders123 commented 1 year ago

Ok, too big to reference and cherry pick for the context here; Checkout the 'No Never' commit mentioned above, 8f1ceef, for a big, way too long, in-depth explanation as to why unknown can't be used as the default for the API's generics.

TLDR: Gonna go back to using any to make sure things are consistent across the board for the user. The other option is for the generic to fallback to RootTag and/or CompoundTag where applicable, but both will allow you to access hallucinated values from the index signature (that's it's nature), and I think that's less straightforward to expect than seeing any, where you know ahead of time it will do that.

For new users, if they see RootTag, and it shows instance methods on there, they will likely want to try to use them, which makes sense. If you see a generic, and any is it's default, it's probably more obvious that it's meant to be filled in by you, the user, when you know the shape of the data ahead of time.

I will work on explaining this in documentation for NBTify once I have it set up.

Thought of a nice overview about the any dilemma: "I think it's safer to be purposefully less explicit, than to provide incorrect or misleading explicit types."

Woop! Looks like I missed a bonus reference from today, in the commit message. Here's that one:

Super cool video here! It's about end-to-end type safety for REST APIs on the server and the client

I think this kind of setup and hearing about tRPC are what are inspiring how the NBT schemas for each version can be set up, where the comparison between how each version is converted/related/translated is hooked together. If you change a part of one version, the other version will show errors if the code for the other version doesn't support the new changes. And the fact that you can create docs from your types too!

https://m.youtube.com/watch?v=tjfEkaPiKQQ

So like the NBT for the players inventory for example should only allow IDs from day a union of all the ID strings for the items in game, say minecraft:bread, minecraft:cake, rather than just being string

So for one version of the game (1.8 per se), the inventory only allows items from the 1.8 item union type. If you try to write a 1.9 inventory structure to a 1.8 game save, it should show a type error

Similar to the one above, but for when we use the schemas to make a world converter between the different platforms/versions

So if we have something random in the 1.8 schema that we want to update, the converter from 1.8 to 1.20 should show type errors where the new 1.8 functionality isn't supported in the 1.8 -> 1.20 converter

Offroaders123 commented 1 year ago

Went back to the original any default across the board, as it works the most understandably both on the type level, and when using the functions and classes in run-time code blocks.

I wrote a bunch more info about this in the commits mentioned before this message, so definitely check those out to see the decision process behind it if you are curious about it.

Offroaders123 commented 1 year ago

Re-added RootTag as the default generic value when not provided with a value to type check against. This is less about how you use it as a type, and more about preventing 'type elision' with other types. RootTag | NBTData<RootTag> is more useful than any | NBTData<any>, because that just flattens to any, which makes working with untyped NBTify-based API structures harder to work with, because you can't access values correctly using those. And you run into errors in runtime that you don't catch in TS! Ouch! @HoldYourWaffle wins with this one 🤓

You can see the process for re-deciding this in the referenced commits above. I ran into this kind of issue while writing the unit test for the SNBT parser and stringifier, and having RootTag as the default definitely helped distinguish between the two types better.

Offroaders123 commented 1 year ago

Decide to try expanding the scope of the generic to allow more values again. I realized this can be a perfect middle ground! The default return value for functions with this kind of generic can still default to RootTag, meaning it doesn't merge with things like NBTData, but now it will also allow you to pass in non index-signatured classes, which it didn't like previously, when using RootTag as the generic filter.