FRONTEND-2309 :: Feature :: Remove getAll and putAll#142
Conversation
doup
left a comment
doup
left a comment
There was a problem hiding this comment.
There few things you could change, but I'm still approving it. Remember the next releas will need to be 2.0, as these are breaking changes. Kind of unfortunate… as we could have waited before doing the 1.0. 😅
| client.clientId, | ||
| client.clientSecret, | ||
| grants.map((el) => el.grant), | ||
| grants.map((el: OAuthClientGrantEntity) => el.grant), |
There was a problem hiding this comment.
Nit: is this type annotation needed? Isn't correctly inferred from GetDataSource<OAuthClientGrantEntity[]>?
| grants.map((el: OAuthClientGrantEntity) => el.grant), | |
| grants.map((el) => el.grant), |
There was a problem hiding this comment.
@doup Agree, but in a discussion in socialPals @lucianosantana told he preffers that we put explicitly all the types so I'm used to it. We could raise this discussion to MJ.
There was a problem hiding this comment.
Technically, is not needed. The type can be inferred quickly on a modern IDE context, but needs a bit of code reading in "dumb" contexts such as browser debugging. I don't think we lose anything by adding that info explicitly.
There was a problem hiding this comment.
I don't have a problem adding it, but it's true that in a dumb context having an OAuthClientGrantEntity you don't have more information than before, and what we have in the end is the logic and algorithms filled with types. My preference is mainly having everything typed but as compact and readable as possible.
Being a library as it is, I'll leave it with these types in there.
There was a problem hiding this comment.
Then this is ready to be merged, right?
| ); | ||
|
|
||
| grants = grantEntities.map((el) => el.grant); | ||
| grants = grantEntities.map((el: OAuthClientGrantEntity) => el.grant); |
There was a problem hiding this comment.
Nit: same here.
| grants = grantEntities.map((el: OAuthClientGrantEntity) => el.grant); | |
| grants = grantEntities.map((el) => el.grant); |
|
@doup The v1.0.0 was really needed in a project so I had to release it. There's nothing wrong on creating a new major release. I had this into account, although we can keep this changes without a new release for now. Also think that all your suggestions are for things that are not created by me in this PR, just moved from the original place, so some applies no only to the place you pointed out but many others. For these cases I'll create an asana. Thanks! |
5 participants
Asana task link:
https://app.asana.com/0/1109863238977521/1204203716532309/f
Pull request information: