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

feat(entity-generator): added ability to output type option in decorator #4935

Merged
merged 1 commit into from Nov 17, 2023

Conversation

boenrobot
Copy link
Collaborator

@boenrobot boenrobot commented Nov 17, 2023

I tried to run the tests with @swc/jest (yes, I did saw #3200, but I thought I take a fresh crack at it, since that branch is over a year old...), and an interesting error that popped up was the inability to discover the entity type.

Featuring the type is one way to avoid this... another way is to not use SWC or to use EntitySchema (which is the solution I intend to use for my projects personally...).

So... for users who intend to use SWC, and would like to use decorators, I think it would be very beneficial to be able to generate the "type" option. For everyone else, it would just provide a minor boost in startup performance of a project using generated entities with decorators.

Being a very limited option in scope, implementation was easy. Honestly, whether this should be an option and whether it should default to true or false is debatable... but I thought I just set it as an option defaulting to false for the sake of BC to avoid this debate... I mean, I think it's OK for it to not be an option and just always default to true... but that requires modification in existing tests if nothing else.

Side note: Because of the way SWC works, I fear the only way to possibly switch to it would be to support BOTH @swc/jest and ts-jest in all tests but those that require reflection... and in CI, run those that require reflection separately with ts-jest while most others would run in @swc/jest.

@B4nan
Copy link
Member

B4nan commented Nov 17, 2023

I tried to run the tests with @swc/jest (yes, I did saw #3200, but I thought I take a fresh crack at it, since that branch is over a year old...), and an interesting error that popped up was the inability to discover the entity type.

I still don't get it, I actually rebased that branch a month ago, and it's still the very same story, it does not bring any speed improvements over ts-jest with isolated modules (so no type checking). So not interested in switching away from what works fine without any added benefits.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, lets just simplify the tests so they can be faster, i also dont think we need to test multiple drivers here as the changes are not driver specific

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c5f4ab4) 99.43% compared to head (4cb1624) 99.43%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4935   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         222      222           
  Lines       16018    16027    +9     
  Branches     3840     3845    +5     
=======================================
+ Hits        15927    15936    +9     
  Misses         88       88           
  Partials        3        3           

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

@B4nan B4nan merged commit 2d1936a into mikro-orm:master Nov 17, 2023
11 checks passed
@boenrobot boenrobot deleted the scalarTypesInDecorator branch February 10, 2024 14:25
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.

None yet

2 participants