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

fix(core): detect path from decorator for each class only once #5545

Merged
merged 2 commits into from
May 28, 2024

Conversation

Ruby184
Copy link
Contributor

@Ruby184 Ruby184 commented May 6, 2024

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 using Object.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:

import { Entity, MetadataStorage } from '@mikro-orm/core';

@Entity()
class User {
  static [MetadataStorage.PATH_SYMBOL] = import.meta.filename // or __filename for commonjs

  @PrimaryKey()
  id!: number;

  @Property()
  username!: string;
}

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.74%. Comparing base (271ff7d) to head (9546e07).
Report is 6 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@B4nan
Copy link
Member

B4nan commented May 6, 2024

Moreover, as a side effect, this also allows defining a path on the class by the developer

This is great, I've been thinking about introducing this for ages. We should document this somewhere.

@Ruby184
Copy link
Contributor Author

Ruby184 commented May 10, 2024

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 import.meta.filename or __filename is scoped to a module (file), and we expect the path to be defined for each class because it usually differs. It's not worth making logic to detect if we can inherit it from the parent, maybe if it is defined as a getter, but we can document that. I am open to suggestions.

@B4nan
Copy link
Member

B4nan commented May 10, 2024

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.

@B4nan
Copy link
Member

B4nan commented May 10, 2024

I guess this could go to https://mikro-orm.io/docs/metadata-providers.

Also, do you think this should be a perf commit? Does it have a measurable difference? Maybe a fix would be better, or even a feat since we are adding a new feature that will have its documentation.

@B4nan
Copy link
Member

B4nan commented May 10, 2024

Btw does this help with the production build issue in bun mentioned in that discussion?

@B4nan
Copy link
Member

B4nan commented May 28, 2024

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.

@B4nan B4nan changed the title perf: detect path from decorator for each class only once fix(core): detect path from decorator for each class only once May 28, 2024
@B4nan B4nan merged commit 9af0e38 into mikro-orm:master May 28, 2024
11 checks passed
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