kysely-org / kysely

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

ESNext build breaks React Native app because of required Babel plugin #67

Closed petrovica closed 2 years ago

petrovica commented 2 years ago

Hi! First of all, I haven't seen such a great idea how to use new TypeScript features in long time. Great idea and even better execution. I wanted to switch from string literal based SQLite query builder to kysely, but I'm having issues making it work on React Native. I'm using new node-less version. That helped a lot (thanks btw!), but I hit another issue. Same as the person here: https://stackoverflow.com/questions/69922302/react-native-flatlist-undefined-is-not-an-object-evaluating-props-getitem

Basically the issue is we need to enable new language features through Babel to support private class props. But that Babel plugin somehow breaks React Native's FlatList component. Well, that's the obvious part, there might be more things broken.

My idea how to fix it was to just transpile kysely to older ES, ES2021 to be precise. And it worked! There's one issue. Kysely's constructor (in src/kysely.ts) doesn't call super as first statement and because of that transpilation to ES2021 fails. For ESNext it does not matter, TS is fine with calling super at the end, but for ES2021 it's not possible when using private props.

Diff to show what I'm talking about: https://github.com/koskimas/kysely/compare/master...petrovica:es2021?expand=1 So one fix would be not using private props, another fix would be to reorganize the constructor to call super first.

Would you be ok with transpiling the code to ES2021? I don't think I'll be able to fix the FlatList component in React Native or that Babel plugin that breaks it. I haven't found any obvious weird stuff that would explain this. But fixing it by using different transpilation target in kysely is a thing I can manage :crossed_fingers: :slightly_smiling_face:

What do you think about the issue and possible fixes? I can't find much more about this issue except for the SO question. It's really an edge case :man_shrugging:

I'm using latest version of React Native (0.67).

koskimas commented 2 years ago

Thanks 🍻

Sure, we can transpile to ES2021 if I can figure out how to fix the constructor. We can't create multiple instances of those classes (the ones created in the else branch of the constructor) so I don't really know a clean way to fix the constructor (a bunch of hacks come to mind though).

The link to the diff doesn't work, don't know if you had a solution there or not.

petrovica commented 2 years ago

Link to the commit: https://github.com/petrovica/kysely/commit/c5d034f1c8c12f3a5bb75e6ae9ee271814e66b0e Hopefully this one works.

No real solution, I just tried getting rid of that private prop to verify it's the only cause of the issue. Once I made this.#props public, I was able to transpile to ES2021 and that fixed the React Native issue.

Yeah, the logic before super call makes it non-trivial, but I'm glad you'd like to move forward with that. Thanks! I'll try to figure something out too :slightly_smiling_face: . If fixing the constructor won't be possible without breaking changes, making this.props public would work too, but I don't like that either :man_shrugging:

koskimas commented 2 years ago

This works, but **ck is it ugly:

const propsCache = new WeakMap<KyselyConfig | KyselyProps, KyselyProps>()

function init(args: KyselyConfig | KyselyProps): KyselyProps {
  if (propsCache.has(args)) {
    return propsCache.get(args)!
  }

  if (isKyselyProps(args)) {
    propsCache.set(args, args)
    return args
  }

  const dialect = args.dialect

  const driver = dialect.createDriver()
  const compiler = dialect.createQueryCompiler()
  const adapter = dialect.createAdapter()

  const log = new Log(args.log ?? [])
  const runtimeDriver = new RuntimeDriver(driver, log)

  const connectionProvider = new DefaultConnectionProvider(runtimeDriver)
  const executor = new DefaultQueryExecutor(
    compiler,
    adapter,
    connectionProvider,
    args.plugins ?? []
  )

  const props = {
    config: args,
    executor,
    dialect,
    driver: runtimeDriver,
  }

  propsCache.set(args, props)
  return props
}

export class Kysely<DB>
  extends QueryCreator<DB>
  implements QueryExecutorProvider
{
  readonly #props: KyselyProps

  constructor(args: KyselyConfig)
  constructor(args: KyselyProps)
  constructor(args: KyselyConfig | KyselyProps) {
    super({
      executor: init(args).executor,
    })

    this.#props = init(args)
    propsCache.delete(args)
  }

 ...
}
petrovica commented 2 years ago

I had to dig a bit in the code to undestand what's going on. You're using this.#props in Kysely and also QueryCreator, so it needs to by private. And executor needs to be shared between Kysely and QueryCreator, but you don't want to move the whole logic to QueryCreator.

It's actually pretty elegant considering the limitations :+1:

koskimas commented 2 years ago

I didn't realize that ES2021 doesn't have private properties and typescript emulates them using WeakMaps. That dropped the query building performance to 50% of the ESNext version. I don't think I'll want to sacrifice that much over React native support right now.

petrovica commented 2 years ago

Makes sense :+1: . Anyway, thanks for your time :slightly_smiling_face:

koskimas commented 2 years ago

Let's hope React native world catches up with the new features soon 🤞

petrovica commented 2 years ago

I don't think it will. Facebook's plan is to switch from WebKit's JS engine to their custom engine (Hermes). It's an amazing upgrade performance-wise, but it currently supports only most of ES6 features. At least we have Babel :slightly_smiling_face: