EventStore / EventStore-Client-NodeJS

A NodeJS client for Event Store
https://eventstore.com
Apache License 2.0
162 stars 21 forks source link

Add Runtime Validation Spike #349

Closed w1am closed 4 months ago

w1am commented 10 months ago

Changed: Integrate Zod for Stronger Type Checking

This PR aims to enhance type safety and data integrity, ensuring our application handles data more robustly and reduces the likelihood of runtime errors due to type mismatches. Additional validations include checking UUIDs and ensuring certain numbers fall within specified limits.

linear[bot] commented 10 months ago

DEV-197 Add runtime validation

yordis commented 8 months ago

I am curious to learn more about who is asking for this and what problems they are running into and why. I would appreciate context since Non-ESDB peeps do not get to have access to Linear to understand more about the linked ticket.

I prefer lean clients that are extremely simple, and definitely would appreciate if EventStoreDB doesn't rely on zod or external packages for something trivial like the validations being done. Especially if they are gonna be performance in a C# guard styles and filtering error list to a single value in the try-catch.

Maybe the solution is better documentation or typespec or even better, introduce Value Objects to enforce correctness.

w1am commented 8 months ago

I am curious to learn more about who is asking for this and what problems they are running into and why. I would appreciate context since Non-ESDB peeps do not get to have access to Linear to understand more about the linked ticket.

I prefer lean clients that are extremely simple, and definitely would appreciate if EventStoreDB doesn't rely on zod or external packages for something trivial like the validations being done. Especially if they are gonna be performance in a C# guard styles and filtering error list to a single value in the try-catch.

Maybe the solution is better documentation or typespec or even better, introduce Value Objects to enforce correctness.

Thanks for the input. Some people were experiencing runtime errors, and we were thinking zod would be a great plug-and-play solution to that. However, I definitely need to conduct some benchmarking before applying this change.

yordis commented 8 months ago

Thanks for the input. Some people were experiencing runtime errors

Is there any context about their environments and what exactly was the problem? Are they doing type checking of any kind? How many times a given team made the mistake vs. newbies that weren't familiar with the stack? Are they copy+pasting code or do they have a slim wrapper around ESDB client? Could they have their own validation layer in their end if they really want "Safety"?

I would love to understand and have more public information about what drove the decision here before we all pay the cost because of "some" users. I do not want to dismiss their experience, while also make sure a layer such as ESDB client is lean, simple and performance.

w1am commented 8 months ago

Is there any context about their environments and what exactly was the problem?

We had 2 issues in the past where someone tried to append to a stream with an invalid UUID, and another case when someone tried to append to a stream with an invalid 'expected revision'.

How many times a given team made the mistake vs. newbies that weren't familiar with the stack

I am not sure about their skill level or their familiarity with the stack. This small PR is more of a developer experience improvement, and I left it in draft for experimenting. Also, if we were to proceed with this solution, users could continue to use the functions as before without many changes.

yordis commented 8 months ago

We had 2 issues in the past where someone tried to append to a stream with an invalid UUID, and another case when someone tried to append to a stream with an invalid 'expected revision'.

That is a really low instances. What would the server do in those cases?

I am not sure about their skill level or their familiarity with the stack.

With low instances and without much understand of the situation, I would prefer if ESDB isn't reactionary in such situation and seek to honestly comprehend what was the root caused of it.

users could continue to use the functions as before without many changes.

Well, sort of, I wish it was that easy.

It will introduce a new external dependency, which in the worst case, tools like package manager will try to dedupe the dependency and may cause potential issues in production.

Or taking into consideration Zod accidental/unintentional breaking changes, security problems and so on, the external dependency problems becomes our problems now.

I would expect ESDB to be conservative with the external package dependencies, especially in the JavaScript ecosystem and npm.

yordis commented 8 months ago

Maybe this SDK could follow the lead of other strong type languages and introduce classes/JS objects/Value Objects where you get to validate the integrity of the data in the constructor instead of relying on types from TypeScript.

yordis commented 8 months ago

@w1am sorry I comment again, just that yesterday while I was trying to fall sleep, I remember another way.

Instead of using class and new keyword, you could make factory functions, for example, makeReadStreamOptions({}), where people opt-in to leverage those functions if they want to enforce correctness in the client side.

This way, the type system could stay the same and does not have to rely on "classes" but just types.

w1am commented 8 months ago

you could make factory functions, for example, makeReadStreamOptions({}), where people opt-in to leverage those functions if they want to enforce correctness in the client side.

Yes, that's a possibility. I'll keep that in mind when making the decision.