-
-
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
feat(entity-generator): allow custom types for scalar relations #5435
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5435 +/- ##
========================================
Coverage 99.74% 99.74%
========================================
Files 261 261
Lines 18060 18096 +36
Branches 4395 4184 -211
========================================
+ Hits 18014 18050 +36
Misses 46 46 ☔ View full report in Codecov by Sentry. |
@@ -20,6 +20,7 @@ export class Address2 { | |||
", | |||
"import { Cascade, Collection, EagerProps, Embedded, Entity, Formula, type Hidden, Index, ManyToMany, ManyToOne, OneToMany, type Opt, PrimaryKey, Property, Unique } from '@mikro-orm/core'; | |||
import { Book2 } from './Book2'; | |||
import { CustomBooleanType } from './CustomBooleanType'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this CustomBooleanType
defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as the entity generator is concerned, that's none of its business 😆. The extension author is expected to have defined it previously, specify the reference to it in an extension hook, and override fileName to resolve the referenced class.
In this case, it's not actually defined (just referenced in the extension hook), and the import is resolved to being in the same folder as the entities.
I expect in practice that people would define their custom prefix/suffix, and override fileName to resolve classes with that prefix/suffix to another folder... So something like fileName: (className) => className.endsWith('Type') ? `../types/${className}` : className
.
The referenced class may also be defined later in a larger script, in which entity generation is just the biggest step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we at least generate a stub for this? like export type CustomBooleanType = boolean
, which users can replace?
as this feels like the only thing that the generator would output and wouldn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... now that you showed this specific stub, I'm thinking there's actually a different can of worms I haven't dealt with... In fact, it's fair to say I didn't think the whole thing through...
We have...
- Type only imports for non-core imports in the prop declaration... The one in the test was meant to be imported and used as such. Custom shapes for plain JSON values (as opposed to embedded JSON values) are perhaps a more realistic example of something that users would want to do, and would still end up having to import as a type import. My
CustomBooleanType
example was instead an example of the second use case for type only scalar imports - a branded type (in this case, a branded boolean, though branded integers are perhaps a more frequent use case). - IType annotated custom types. The above does not qualify as such, but what about types that do? Things like "an email type" for example that validate on input. The meta data would have the type class, but the type class is not what should be in the prop definition (just the prop options instead).
The scope of this PR suddenly increased a bit, but I guess better to address at least one of those two things, if not both...
I'm thinking of trying to do a dynamic import of the referenced identifier (when the ref is a scalar non-enum of a non-lib type...) after letting fileName dictate the import path... if the file does not exist, or the identifier does not exist in it, or exists, but is not a mapped type, assume it is a type only import, and output it as such (kind of like now, except CustomBooleanType
would be imported as type). Otherwise, check runtimeType
and compareAsType()
to determine the value of the prop type. If they are the same, use that for the prop declaration. If not, use IType<${runtimeType}, ${compareAsType}>
, and add an import for runtimeType
(if it's a non-lib type, which is the typical case).
Even if the above approach works, I don't see how to get the serialization type out of the Type, but then again, there's also 0 examples of using it anywhere (not used in any test, not in docs...), so I guess that's fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after letting fileName dictate the import path
This bit was also pretty confusing to me, if we expect people to use this to override import paths, we need to document it somewhere. And I am not so sure it's a good idea.
I feel like you are building something without a clear understanding of how the end users would do stuff. As I mentioned many times, entity generator wasn't meant as a schema-first approach (it was more of a helper instead), which I think you are trying to build. So I would first think about how are people supposed to combine it with their own code, and only then think about those cases. And of course, this all needs to be clearly documented, making code changes without that won't help much. Feels like you are going in the wrong direction, I would design this from the top to bottom.
d7c3bce
to
c4ce860
Compare
3014814
to
c565c2f
Compare
OK, I implemented a completely different approach, which however requires a big change in what the generator produces overall... Basically, we're taking advantage of there being both This produces a very different output in pretty much all entity generator tests though... The value of "type", where present, is now the type in the types map, rather than the runtime type, and this change also comes with the perk of columnType being needed less often. I've added to the docs a very (perhaps overly) detailed explanation on how types are handled, to aid meta data extensions, with some examples. Let me know if I should copy it over to 6.2, or if, due to this being somewhat of a breaking change for extensions (due to "runtimeType" now being used in many places instead of "type"...), maybe not, and keep this only for the "next" (6.3?) version. Even with this more comprehensive change, the extra If you think using |
c565c2f
to
ef0cb5b
Compare
This is done by fully refactoring type inference in general, to take advantage of "runtimeType" in the prop, combined with more cross checks to add IType where needed. Also removed the accidental Embedded decorator in EntitySchema.
ef0cb5b
to
6a6b23b
Compare
This is done by fully refactoring type inference in general,
to take advantage of "runtimeType" in the prop,
combined with more cross checks to add IType where needed.
Also removed the accidental Embedded decorator in EntitySchema.
Using custom types for scalar relations was sort of possible even before this, but it has always been more hacky than anything else. With this, it is actually encoded as an expected possibility, and added to the test for hooks. Users who want to use custom types will need to handle their resolution in the fileName function. Same as if those identifiers were referring to an entity.