multiformats / js-cid

CID implementation in JavaScript
MIT License
98 stars 39 forks source link

fix: relax version type #129

Closed achingbrain closed 3 years ago

achingbrain commented 3 years ago

We use the TS typing to enforce only 0 or 1 as version numbers, but mark the version property of a CID as number which means it can be any value.

Here we relax the version number input to be the same as the version property which means:

a) we aren't using types for data validation any more (please let's not do this) b) we can pass cid.version in to other functions without needing to do weird type contortions to account for it only being 0|1 c) we can handle CIDv2 if/when that comes along

Gozala commented 3 years ago

we aren't using types for data validation any more (please let's not do this)

I'm not sure I understand what you mean here.

we can pass cid.version in to other functions without needing to do weird type contortions to account for it only being 0|1

Just a side note 0|1 is subtype of number so it will be valid number and should not be a problem. It does not work other way round though.

c) we can handle CIDv2 if/when that comes along

When we add support for CIDv2 it would be great time to change type to 0|1|2. In fact the main reason to use types like 0|1 is to help with those kind of things.

Gozala commented 3 years ago

I just made a sketch that solves version field problem & #128 https://www.typescriptlang.org/play?#code/PQKhCgAIUhhAbAhgZ2ZATgUwA5eZgOwBcBLAgc0kTgEkARSAAwB4BbAIxUwD5mA3TOmQkA9gV6sAxiIAmmSRIAWKRd0ZQYAGipo5AMzKYZkMpADaJbPBnBJJGQF0AFIqJFsyAFzBg5EkUUAV3YAOmlWYFZA+FI9EXRWRCJkW3sAShCNYHA5SSQsSDyUNFh6SABvKEhQCEg6mFgsJMwqSAJMAHdaOky66CroSAAVRRbEeHJ4-0VWSDj0KnRyQNZCIhMCbED1kjR0EUDyRXgAT08BmEYri5M9SCc7GTSbuoBabkKmojHCkWwTm6YeD4W73ZBEdDPPowOokO5OACM4MKygWu0gYkgURiJE4IKIiHY8EwaUg726kAAyhCyOQXpAgSDyexkABWAAcDII0jkxmxpGUyEUgOBLTh9wAqmQiOyAILodCIE5Q+p9cWI5HsE7fExoAAMGIWCNJ5NKdHpjJa5P5JEFwuhDNFoKcADkVuxBCrBm8PtICOD0IFJOszZAtZBsIh0Mkbld1NCbgABTAAD0QrCsmBu7S6Zqc-EEwjE3G0zB58hLkDY0QFKkr1ZxeMwLvTPC9MBz3Xzj2p6G47banS7ZfsIS132Q-ezQ7zDdrQqnDs7s5ZHK55b5NdtdYHy-o3fsi-q2TqfoDQaI8ScA0gAiEogInm6ADVCw-NDfy5InwHaZAAD5tO6ggfn0WI1naT5SsQcoKkqoF9DaTYtqsAD8P40hQVRpAA3FUZ4QheV6PE+Zr-r+FD-tBMryoqyp4VUNQ3CMLR3kWBAYncAQtGavTHlUTQyGIpy3m+YikfQr73mIDF1ExDosb8uScZA3HdHx0AnhgmCIEJBAiV+GHoLSsnVGAzGjEp8gqWpoamP4aAEMBCzzKwGkgFpgnCScVlPk5HCCKZ8mqopNp2jZlm8Rcnk6XpIlhSoUHSrBdFBeZDoALJbk2bStjokAUeQ7kxbp3ngY2XAoZgRkmeAjHpSFkVlCgrSMNRKVKvG-F1F5+k+eOmBeJA7W0UqaW1I1LS4JgBgphFPH0MVAmxWV02zUlMGjSc403LAYh3usl6tKGIh3GxD5MHqjBLXUl7PnqThpBJdA7Q6e0EAdqkiMdZSnaJ0kcYwCLXdFVR3Qij3Pa9qoAKLcrILS2WU0rfdQhXuUmkaKrMOUAEJcOush-kdgT4DdX34-gcMbr2tJOE26EFZh5BPUzxkUNDgzUwjqlNQwKOtOjoO3SItMUPTXCM4VrOFZzMCUoIJDjCQABeiOoxGSCmCI7AAFbyEQ5OXgAUpSADyLqQxUVnfmzJn-exT4GoBCI4ZAkHDclW2QAAvnLcAiBmUYtJgACOgTK0QPkdNMVAECI3ELFFCYOomWPphiifDJZCejEni3Cwy4fjMgTi54IT6IAQypPuwIgiMSVf+0Mg07Fxll+AIHFkFsOxoNQfDK8YJ16wb7nZ-sHRoOK-i6m0CcY6n6ezOXCyKav6mF+CSQkJIt5D80ear5X1es3wIj2KZ2+kHvux5qwJApkYJ811ij9GHPZpXwSN820NABK8h4gyGYIVbQ-kPR9jwj7OqUdsALToFJdikAAC8kA9T-gRHVAicx9izDQQ8ewnhCpUS9nBE4-4zSkhQR8PcdBCFPHADgrGIICEkWlqg2hM59yPGeDgvIYgWhsKIVQzhg5cw8PSEwsQyJpD-FQfcEioiaHiK7I8EI50xDaHUV+bRo4EpCj4TI9YuQebCJkJ4Ea5DqFcIkfQ3h0j-TrEkF8IR9xNGPggSBG2flnLaAMYoDaNFrFiMqLCeEHjUEoLQc7QCkToloONNbG8WAiCBHQBxOhTgPHaIRpIfxW47Qqh9o6EEYSwK80npAGGCorwAHIaAfSHg7B8dTingB9kAA

achingbrain commented 3 years ago

I'm not sure I understand what you mean here.

What I mean is, 0, 1, 2 are values and we're using the type to constrain the value, sort of like an enum, except this is a version number which doesn't seem like the sort of thing you'd use an enum for.

rvagg commented 3 years ago

Is there an existing code pattern that the constraint to 0|1 is causing problems for? I can imagine one, but I can't imagine a realistic one where a CID version number is coming from an unconstrained number that might have some other use. Or are we just wanting to give API consumers more flexibility so that, in the eventuality of a 2, it doesn't get extra-hard?

achingbrain commented 3 years ago

Is there an existing code pattern that the constraint to 0|1 is causing problems for?

Yes, because .version is less restrictive type you cannot use it as the first argument to the CID constructor:

const cid1 = new CID('Qmfoo')
const cid2 = new CID(cid1.version, cid1.codec, /* ..etc */) // not allowed

Though it looks like #130 has approached this by going the other way and making it more restrictive.


Anyway my point is, a version number is not a type - it cannot be subclassed for polymorphism, nor does it define or possess an interface that can be used for abstraction. It's just a number, which is a type.

That we only support 0 and 1 as values for the version number is a coincidence and enforcing that with a type system seems wrong.

Gozala commented 3 years ago

Anyway my point is, a version number is not a type - it cannot be subclassed for polymorphism, nor does it define or possess an interface that can be used for abstraction. It's just a number, which is a type.

In algebric data types that is not exactly true and typescript type system does qualify as algebraic, where you can think of number a union of all possible numbers. And in algebric data types you can refine types by narrowing them down (e.g. with switch or if statements in TS). You can even subtype (e.g number & string would be a supertype, it can’t be instantiated but sometimes that is useful in fact that is more or less what never type is).

That we only support 0 and 1 as values for the version number is a coincidence and enforcing that with a type system seems wrong.

I would suggest following view instead

In more practical terms program that wishes not to fail should either enclose call to CID(verison, ... in try { } or ensure that version is 0 or 1 before passing. Encoding version as 0|1 makes that explicit.

Gozala commented 3 years ago

Closing since we decided to go with alternative proposal & revisit if it will end up to be a pain.