Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why freeze? #20

Closed
steida opened this issue Nov 20, 2021 · 6 comments
Closed

Why freeze? #20

steida opened this issue Nov 20, 2021 · 6 comments
Labels
question Further information is requested

Comments

@steida
Copy link

steida commented Nov 20, 2021

I am just curious, why freeze? Isn't TypeScript readonly enough? I remember some issues related to freeze automerge/automerge#177

@koskimas
Copy link
Member

koskimas commented Nov 20, 2021

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
Copy link
Member

koskimas commented Nov 20, 2021

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
Copy link
Author

steida commented Nov 20, 2021

OK, thank you!

@steida steida closed this as completed Nov 20, 2021
@steida
Copy link
Author

steida commented Nov 24, 2021

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
Copy link
Member

koskimas commented Nov 27, 2021

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
Copy link
Author

steida commented Nov 27, 2021

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

@igalklebanov igalklebanov added the question Further information is requested label Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants