feat: type resolver for property decorators #1751
Conversation
| * Determines whether the given constructor is a custom type or not | ||
| * @param ctor Constructor | ||
| */ | ||
| export function isComplexType(ctor: Function) { |
There was a problem hiding this comment.
Moved to @loopback/repository as isBuiltinType.
| }); | ||
| } else { | ||
| Object.assign(propDef, {$ref: `#/definitions/${resolvedType.name}`}); | ||
| } |
There was a problem hiding this comment.
I re-organized the branches in a different order, that's why the diff looks bigger than the changes actually are.
| resolveType(metaProperty.itemType as string | Function) | ||
| : resolvedType; | ||
|
|
||
| result.definitions[referenceType.name] = propSchema; |
There was a problem hiding this comment.
I added a reverse check + continue, this part is best reviewed with white-space changes ignored. (See "Diff settings" at the top of the page.)
| // populating "properties" key | ||
| result.properties[p] = metaToJsonProperty(metaProperty); | ||
|
|
||
| // handling 'required' metadata |
There was a problem hiding this comment.
Moved here from the bottom of the loop body to allow me to use early continue.
b-admike
left a comment
b-admike
left a comment
There was a problem hiding this comment.
left some minor comments, but LGTM 👍
| expect(isTypeResolver(Boolean)).to.be.false(); | ||
| }); | ||
|
|
||
| it('returns false when the arg is Object type', () => { |
There was a problem hiding this comment.
Nitpick: since the logic in these tests is the same, can you create a helper function which takes in the parameter to the isTypeResolver function and creates a test case with the type name as well as the expected result?
There was a problem hiding this comment.
Creating parameterized test cases is tricky. If done wrong, then test failures don't point to the source code of the individual test case, only to the same helper function. In coercion tests, we found a solution where we intercept errors and append extra stack trace to them in order to preserve location of where the test case was defined.
In this particular case, I think such extra complexity outweighs benefits.
| expect(inst.toISOString()).to.equal(A_DATE_STRING); | ||
| }); | ||
|
|
||
| it('supports Date provider', () => { |
There was a problem hiding this comment.
Nitpick: would supports Date resolver be better than provider?
| expect(isBuiltinType(Number)).to.eql(true); | ||
| }); | ||
|
|
||
| it('returns true for String', () => { |
There was a problem hiding this comment.
Nitpick: ditto re organizing the test cases in a clear way, while utilizing a helper function like I mentioned above. With that said, I'm still fine with these explicit tests.
| } | ||
|
|
||
| /** | ||
| * Check if the provided function is a built-in type: a constructor-like |
There was a problem hiding this comment.
Nitpick: can we say built-in type or a constructor-like... instead of using a colon?
Implement a concept of deferred type resolution, where the decorator metadata provides a TypeResolver function to obtain the actual type to use. For example: ```ts @Property(() => Product ``` Deferred resolution is a solution for avoiding require() loops that are frequently encountered when defining relations like hasMany + belongsTo. Co-authored-by: Kyusung Shim <ks1771@naver.com>
bajtos
force-pushed
the
feat/property-type-resolver
branch
from
a9f4387 to
023e6d0
Compare
Implement a concept of deferred type resolution, where the decorator metadata provides a TypeResolver function to obtain the actual type to use. For example:
Deferred resolution is a solution for avoiding require() loops that are frequently encountered when defining relations like hasMany + belongsTo.
I have extracted this pull request from #1618, please refer to that pull request to better understand the context of this change and previous discussions.
In #1618, I was advocating against using
Type | TypeResolverunion. As I was thinking more about other approaches, the impacts on developer experience (LB4 app authors) and how little time we have until 4.0 GA, I decided to continue down the road started by @shimks.This pull request is related to #1361.
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated