Skip to content

Prisma Kysely v2.0.0 #110

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

Open
wants to merge 52 commits into
base: main
Choose a base branch
from
Open

Prisma Kysely v2.0.0 #110

wants to merge 52 commits into from

Conversation

arthurfiorette
Copy link

@arthurfiorette arthurfiorette commented Oct 11, 2024

closes #91
closes #88
closes #69
closes #96
closes #106
closes #115
closes #116
closes #117
closes #94
closes #97
closes #100

Previously, this was a PR created to introduce groupBySchema. However, following recent discussions on Kysely discord, this PR now is home to all work for the upcoming prisma release v2.0.0

Old description

closes #91

I didn't need to fix any test after adding this feature, this means that everything kept running exactly as before when groupBySchema is not true.

We are using this plugin in a giant schema (100+ tables under 18 schemas) where some of them have the same or similar names, which is very hard to spot visually which schema an imported type comes from.

This plugin simply adds all enums and models under a ts.ModuleDeclaration respective to their schema and fixes types references to it. It makes it much easier to work with schemas, the same visually/architecturally as thinking about why schemas are written like schema.model inside SQL.


EDIT: I also added filterBySchema, which just picks all models from defined schemas.


generator kysely {
    provider        = "node ./dist/bin.js"
    previewFeatures = ["multiSchema"]
    groupBySchema   = true
}

datasource db {
    provider = "postgresql"
    schemas  = ["mammals", "birds", "world"]
    url      = env("TEST_DATABASE_URL")
}

model Elephant {
    id      Int     @id
    name    String
    ability Ability @default(WALK)
    color   Color

    @@map("elephants")
    @@schema("mammals")
}

model Eagle {
    id      Int     @id
    name    String
    ability Ability @default(FLY)

    @@map("eagles")
    @@schema("birds")
}

enum Ability {
    FLY
    WALK

    @@schema("world")
}

enum Color {
    GRAY
    PINK

    @@schema("mammals")
}

Now generates:

export namespace World {
  export const Ability = {
    FLY: "FLY",
    WALK: "WALK",
  } as const;
  export type Ability = (typeof Ability)[keyof typeof Ability];
}
export namespace Mammals {
  export const Color = {
    GRAY: "GRAY",
    PINK: "PINK",
  } as const;
  export type Color = (typeof Color)[keyof typeof Color];
  export type Elephant = {
    id: number;
    name: string;
    ability: Generated<World.Ability>;
    color: Mammals.Color;
  };
}
export namespace Birds {
  export type Eagle = {
    id: number;
    name: string;
    ability: Generated<World.Ability>;
  };
}
export type DB = {
  "birds.eagles": Birds.Eagle;
  "mammals.elephants": Mammals.Elephant;
};

Instead of putting everything globally as previously:

export const Ability = {
  FLY: "FLY",
  WALK: "WALK",
} as const;
export type Ability = (typeof Ability)[keyof typeof Ability];
export const Color = {
  GRAY: "GRAY",
  PINK: "PINK",
} as const;
export type Color = (typeof Color)[keyof typeof Color];
export type Eagle = {
  id: number;
  name: string;
  ability: Generated<Ability>;
};
export type Elephant = {
  id: number;
  name: string;
  ability: Generated<Ability>;
  color: Color;
};
export type DB = {
  "birds.eagles": Eagle;
  "mammals.elephants": Elephant;
};

@arthurfiorette
Copy link
Author

arthurfiorette commented Jan 10, 2025

I've published this PR as @arthurfiorette/prisma-kysely@1.9.1 if anyone wants it

@Bathlamos
Copy link

Bathlamos commented Feb 4, 2025

Thank you for taking the time! I tried your implementation on a schema with a hundred tables and it works fine. ESLint complains about the use of namespaces. I want to note that they aren't strictly necessary as Prisma already forces the model names to be distinct even if they are in different schemas. Another interesting avenue of improvement might be to designate a schema as default, e.g. the public schema, to avoid prefixing its models.

@arthurfiorette
Copy link
Author

Hey @Bathlamos, I just added a defaultSchema option!

@alii alii self-requested a review May 17, 2025 03:06
@igalklebanov
Copy link

igalklebanov commented May 17, 2025

Hey 👋

I think that the use of namespaces should be replaced with a file (module) per PostgreSQL schema.
If people still want that namespace syntax when consuming, they can just:

import * as Public from 'path/to/public/schema/types'
import * as MySchema from 'path/to/my-schema/schema/types'

Public.Person
MySchema.Person

More shorter files = better performance as seen with https://www.prisma.io/docs/orm/prisma-schema/overview/generators#output-splitting-and-importing-types.

Each schema module could expose it's own Database interface:

export interface Database {
  'my_schema.person': Person;
  'my_schema.pet': Pet;
}

This way, you could create a Kysely instance that only queries my_schema with ease, while each schema module's Database could help build the main Database interface in the "root" file.

@arthurfiorette
Copy link
Author

In general, * as Public and other forms of non-named exports don't work well with IntelliSense or auto-imports, unless we manually emit both JS and DTS files using the export as namespace syntax. However, doing so would significantly deviate from the current plugin architecture.

Also, the output generated by prisma-kysely differs from Prisma in that Prisma emits a large number of complex type definitions per model. In contrast, prisma-kysely provides a straightforward 1:1 mapping. So unless we're dealing with thousands of database models, performance shouldn't be a concern here.

Namespaces are often considered problematic when declared across multiple files, as that can make it harder for the TypeScript type checker to do its job efficiently. However, that doesn't seem to be the case here also.

Given all of the above, I still believe using namespaces is the right approach here.

@igalklebanov
Copy link

igalklebanov commented May 17, 2025

In general, * as Public and other forms of non-named exports don't work well with IntelliSense or auto-imports, unless we manually emit both JS and DTS files using the export as namespace syntax. However, doing so would significantly deviate from the current plugin architecture.

The bottom line is:

The current solution is forcing a specific (and probably niche - the team recommending not to use them and eslint yelling at people in the recommended ruleset for years didn't help namespace's cause) style on everyone with your problem, and causing problems downstream - e.g. eslint yelling at people. This is a DX problem. People might want grouping, but they didn't sign up for namespaces. Grouping can be done in various ways.

So either provide both, e.g.:

type _Table = {...}

export type Table = _Table

export namespace MySchema {
  export type Table = _Table
}

...or allow choosing the flavor of grouping (e.g. turn the boolean into a choice between 'namespaces' and 'modules').

Also, the output generated by prisma-kysely differs from Prisma in that Prisma emits a large number of complex type definitions per model. In contrast, prisma-kysely provides a straightforward 1:1 mapping. So unless we're dealing with thousands of database models, performance shouldn't be a concern here.

It's more about the size of the file, at least according to this, and what I (mis?)heard from the ORM team.

Namespaces are often considered problematic when declared across multiple files, as that can make it harder for the TypeScript type checker to do its job efficiently. However, that doesn't seem to be the case here also.

Another reason to avoid them? 😉

@arthurfiorette
Copy link
Author

arthurfiorette commented May 17, 2025

Hey @igalklebanov!

the team recommending microsoft/TypeScript#30994 (comment)

As far as I understood, that comment points out problems that happens when using namespaces across multiple files. (As the typescript team suffered that personally since TS's own codebase was structured as a few giant namespaces)

It was easy to implement namespaces here because of our current architecture as we can simply reuse everything already made and wrap the result with an ts.factory.createModuleDeclaration().

It also said:

  • "If you have code that you feel needs to use a namespace, more power to you and go for it, but most of the time you don't need one, as we now have a different conceptual organizational model recommended for large-scale JS (modules)"

eslint yelling at people in the recommended ruleset

We have eslint rules to disallow/enforce almost everything. Ideally, generated files should not be linted. Is there something more personal than linting rules?

This is a DX problem.

DX usually means a combination of personal preference and familiarity.

So either provide both.

I'd love to later help you by reviewing a PR adding support your alternative :)

Another reason to avoid them?

The problem said states something that does not happens in this use-case: 1 single file with 1 model per file.


I'll never be against new features but personal preferences does not seems a compelling use case since the current implementation does not seems worse ihmo.

Although using import * as Group and emitting multiple files would require an interesting rewrite of generateFile.ts alongside all the known problems when correctly generating import statements across multiple files for many cjs/esm tsconfig variants.

As far as my knowledge goes into the type checker and compiler internals, enabling isolatedModules makes namespace declarations type-checking behavior almost identical as if types were declared in the file root (Although I might be wrong here).

@igalklebanov
Copy link

igalklebanov commented May 17, 2025

As far as I understood, that comment points out problems that happens when using namespaces across multiple files. (As the typescript team suffered that personally since TS's own codebase was structured as a few giant namespaces)

It also said:

"If you have code that you feel needs to use a namespace, more power to you and go for it, but most of the time you don't need one, as we now have a different conceptual organizational model recommended for large-scale JS (modules)"

This is not about namespaces, but the public perception of them, prominent tooling/voices recommending against their usage (or recommending something else instead), and the majority of TypeScript developers not wanting them in their codebases as a result.

Forcing them, by surprise, as the only way, is not good DX, and is fighting against the grain.

No amount of cherry-picking is going to help the case for them.

e.g. I'm more than OK with string enums, but the ship has sailed. Most people say "enums bad", because that's what they've been fed - no nuance. I understand that I'm in the minority, and I will never force enums on anyone as a result.

We have eslint rules to disallow/enforce almost everything.

But this one is in the recommended preset of a linter that's downloaded 48,854,801 times per week. How many people care enough (about namespaces) to tinker with the recommended preset and allow them?

Ideally, generated files should not be linted. Is there something more personal than linting rules?

How many people configure their tooling properly? How many headaches will this cause?

I'd love to later help you by reviewing a PR adding support your alternative :)

I didn't mean to demand anything. This is just how I talk sometimes. :)

@alii
Copy link
Collaborator

alii commented May 19, 2025

I think namespaces are suitable here

In my opinion I believe it's unfortunate that no-namespaces was a default/recommended rule for the TypeScript eslint plugin. I personally usually turn that one off.

Additionally namespaces are a pretty similar representation in TypeScript of what it means to have a database schema. While separate files do represent some separation to some degree, I would say namespaces are a closer match in terms of what they semantically represent.

Could the solution be that we have a // eslint-disable comment at the top of file? It sounds like the only issue with using namespaces is that people using ESLint will see errors in the file.

@arthurfiorette arthurfiorette changed the title feat: add groupBySchema to group types and enums under a namespace Prisma Kysely v2.0.0 Jun 22, 2025
@arthurfiorette
Copy link
Author

Hey @frannyfx, just letting you know that I copied your #100 PR into this one so we can avoid merge conflicts and ship your fix as fast as possible.

@arthurfiorette arthurfiorette requested a review from alii June 22, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants