kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.44k stars 266 forks source link

Why freeze? #20

Closed steida closed 2 years ago

steida commented 2 years ago

I am just curious, why freeze? Isn't TypeScript readonly enough? I remember some issues related to freeze https://github.com/automerge/automerge/issues/177

koskimas commented 2 years ago

As you know, readonly is just a compile-time concept and doesn't actually guarantee anything. Performance is not an issue. I've run some simple tests and it takes around 0.012 millisecond to build and compile this query to SQL:

      const qb = db
        .selectFrom(['animal', 'person as p'])
        .select('animal.name')
        .distinctOn('p.firstName')
        .whereRef('p.lastName', '=', 'animal.name')
        .where('animal.name', 'in', ['foo', 'bar', 'baz'])
        .whereExists((qb) =>
          qb.subQuery('movie as m').whereRef('m.id', '=', 'p.id').selectAll()
        )

Dropping freeze takes that down to something like 0.010 milliseconds. Both of those numbers are meaningless compared to everything else, like executing the query and sending it to the database server. Both of which take hundreds or thousands of of times longer and are done outside of Kysely.

By using freeze I can be sure nothing is ever mutated by accident.

koskimas commented 2 years ago

Actually, I just ran that test again and it takes around 0.01 milliseconds with and without freeze. Object.freeze has no effect on performance in Kysely anymore.

I ran this on a M1 macbook air and node 16.12.0.

Here's the test if you want to run it yourself:

      function test() {
        const qb = ctx.db
          .selectFrom(['person as p', 'pet'])
          .select('pet.name')
          .distinctOn('p.first_name')
          .whereRef('p.last_name', '=', 'pet.name')
          .where('pet.name', 'in', ['foo', 'bar', 'baz'])
          .whereExists((qb) =>
            qb.subQuery('movie as m').whereRef('m.id', '=', 'p.id').selectAll()
          )

        const result = qb.compile()
      }

      for (let i = 0; i < 1000; ++i) {
        test()
      }

      const N = 100000
      const t0 = new Date()
      for (let i = 0; i < N; ++i) {
        test()
      }
      const t1 = new Date()
      console.log(`${(t1.getTime() - t0.getTime()) / N} ms`)
steida commented 2 years ago

OK, thank you!

steida commented 2 years ago

I still think it's unnecessary with read-only types, but it's probably harmless, except for little DX overhead. I can't imagine a case when freezing would help. Is there a possibility kysely objects are mutated by some other library? I am just curious.

koskimas commented 2 years ago

A good example is third party plugin or dialect that does something like this

const something = somethingImmutable as any
something.someReadonlyField = 42
return something

and then uses that somewhere else to get something done. Then people using that plugin or dialect would open issues here about some obscure bugs that plugin is causing. freeze prevents this kind of hacking.

Also in some rare places Kysely needs to use the any type internally to keep the code sane. For example see the parsers. They wouldn't benefit from strict types that much, but the code would become completely unreadable if we kept the DB, TB and the derivative types everywhere.

In these places where some things are declared as any, things could be mutated by accident causing unpredictable side effects somewhere else.

Since Object.freeze has absolutely no meaningful effect on performance, but makes sure on javscript engine level that nothing gets mutated, why not use it? I understand that the same is true for all other types too. You can cast a number to any and assign a string to it, but there's no clean way to prevent that on javascript level. If there were, I'd use it.

Also in my experience, libraries need to be as "fool proof" as possible. If there is a way to use the API the wrong way, people will find it and hack the hell out of it and then go to stackoverflow/reddit to scream how king it that library is when it's bugging because of the hacks.

steida commented 2 years ago

Thank you for such a detailed answer. I have no other question.

As for "no clean way to prevent that", there is a lib implementing newtype pattern, https://github.com/gcanti/newtype-ts, I was using it, but the ceremony with that was so annoying I switched back to branded types, which can be violated, but that's the responsibility of the developer.

Speaking of branded types, I am using io-ts a lot, so instead of a string or a number, I am using String64 or PositiveInteger etc. It's invaluable for handling untyped values from REST APIs etc.

Just for fun, that how I was trapped by fp-ts. io-ts decode returns Either, and I was furious I can not extract a value "easily" so I had to learn pipe Either map, chain, match etc. 😂

https://github.com/gcanti/io-ts/blob/master/index.md