-
-
Notifications
You must be signed in to change notification settings - Fork 496
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
fix(core): detect path from decorator for each class only once #5545
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5545 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 260 260
Lines 17972 17976 +4
Branches 4370 4372 +2
=======================================
+ Hits 17926 17931 +5
+ Misses 46 45 -1 ☔ View full report in Codecov by Sentry. |
This is great, I've been thinking about introducing this for ages. We should document this somewhere. |
Sure, but there is a caveat. I think people will expect to define the path in the base class and inherit it. However, they need to copy-paste the code for each class, which is slightly counter-intuitive. This comes from the fact that |
Yes, that is surely something to document. I don't think it makes sense to try to validate this, as having the entities in a single file is still a valid case. |
I guess this could go to https://mikro-orm.io/docs/metadata-providers. Also, do you think this should be a |
Btw does this help with the production build issue in bun mentioned in that discussion? |
I'll merge this in as a fix, as I don't want to hold it for no reason, but I would still appreciate response to my previous comment as well as a PR mentioning this in the docs somewhere. |
Fix for bun path detection
Related discussion here: #5496
In this commit you changed the condition because of coverage. This is incorrect because it guards against going out of stack array bounds - I have added the test for this case. Also, I removed no longer needed condition.
Performance
Before the changes, the path was detected for every used decorator on the class and overwritten. This is not needed because the path used as a key should be stable and should not change for a given class. So I introduced the new symbol
MetadataStorage.PATH_SYMBOL
, which will hold the path and check if it exists for a given class usingObject.hasOwn
(this also works for class inheritance so the path is not taken from the base class but defined for each child).Moreover, as a side effect, this also allows defining a path on the class by the developer (maybe in cases like when Bun was not working), but it should not be needed and used only as a workaround: