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

Feature/mikro orm #79

Merged
merged 9 commits into from
Jun 8, 2024
Merged

Conversation

acelaya
Copy link
Member

@acelaya acelaya commented Jun 8, 2024

Closes #50

This PR migrates from TypeORM to MikroORM. Let's use it to enumerate pros and cons.

Pros:

  1. Database connection is established as expected with MikroORM.
    With TypeORM, initializing the connection in the server script didn't work for some reason, forcing a weird initialization in the root.tsx loader funtion.
  2. Entities can be defined as classes, even when using the EntitySchema instead of decorators.
  3. Migrations can be written with knex and they are way simpler.
  4. It has better support for upsert, taking care of internally doing SELECT + INSERT/UPDATE in databases that don't support upsert natively.
  5. It has been created by a formed Doctrine contributor, making it feel closer to the ORM used in main Shlink repo.
  6. Column type definition feels more "abstract" in general, which is good to support multiple database engines.
  7. Entity relationship definitions are more intuitive and less verbose.

Cons:

  1. Running migrations with tsx does not fully work, as MikroORM expects ts-node to be installed.
    This can be worked around defining a couple simple custom scripts.
  2. Running the project with tsx in general has some side effects due to how esbuild resolves transient dependencies.
  3. In MikroORM, driver instances are statically imported from their corresponding packages, which might make it harder to not include all of them by default.

@acelaya acelaya marked this pull request as draft June 8, 2024 07:24
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

Attention: Patch coverage is 77.35849% with 12 lines in your changes missing coverage. Please review.

Project coverage is 70.96%. Comparing base (3675cfd) to head (e641d75).
Report is 17 commits behind head on main.

Files Patch % Lines
app/servers/ServersRepository.server.ts 42.85% 8 Missing ⚠️
app/settings/SettingsService.server.ts 77.77% 0 Missing and 2 partials ⚠️
app/servers/ServersService.server.ts 0.00% 0 Missing and 1 partial ⚠️
app/tags/TagsService.server.ts 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   70.75%   70.96%   +0.21%     
==========================================
  Files          26       25       -1     
  Lines        1019      961      -58     
  Branches      113      106       -7     
==========================================
- Hits          721      682      -39     
+ Misses        185      173      -12     
+ Partials      113      106       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya acelaya marked this pull request as ready for review June 8, 2024 08:38
@acelaya acelaya merged commit f93651b into shlinkio:main Jun 8, 2024
6 checks passed
@acelaya acelaya deleted the feature/mikro-orm branch June 8, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider migrating to mikro-orm
1 participant