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

ObjectTypeLiteral support #415

Merged
merged 9 commits into from
Sep 1, 2019
Merged

ObjectTypeLiteral support #415

merged 9 commits into from
Sep 1, 2019

Conversation

WoH
Copy link
Collaborator

@WoH WoH commented Aug 14, 2019

Currently open issue: #50

R4R

@WoH WoH force-pushed the deepobj branch 2 times, most recently from da71a89 to d84d72f Compare August 22, 2019 22:14
@dgreene1
Copy link
Collaborator

I see that this is still marked as "Draft" Do you want to remove that @WoH or are you still waiting for more feedback from other members or something?

@HoldYourWaffle
Copy link
Contributor

I'm a little confused (probably due to severe lack of sleep) so sorry if this is a stupid question, but what kind of 'objects' can we use know? Is it an implementation of #353?

@WoH
Copy link
Collaborator Author

WoH commented Aug 23, 2019

@dgreene1:

I see that this is still marked as "Draft" Do you want to remove that @WoH or are you still waiting for more feedback from other members or something?

Mostly I'm dreading the tests I'll want to write around this. This PR should already cover nested JSDoc annotations and pretty much everything we support on interface declarations. However, that means we want to make sure it does and will not regress at any point in the future.

@HoldYourWaffle:

I'm a little confused (probably due to severe lack of sleep) so sorry if this is a stupid question, but what kind of 'objects' can we use know? Is it an implementation of #353?

No. The primary concern of this PR are type definitions wrapped inside {}.
See here for reference:

interface Model {
  prop: {
    nested: string
  }
}

or

function(): { value: number } {
  ...
}

@HoldYourWaffle
Copy link
Contributor

The primary concern of this PR are type definitions wrapped inside {}

Wait how is that different from my implementation of #353 (supporting refObjects)? Does this only support 'objects' with a single property?
Again sorry for my stupidity, I just can't get it to click in my head for some reason...

@WoH
Copy link
Collaborator Author

WoH commented Aug 23, 2019

Wait how is that different from my implementation of #353 (supporting refObjects)? Does this only support 'objects' with a single property?
Again sorry for my stupidity, I just can't get it to click in my head for some reason...

Ok, so a refObject is the internal representation for a named interface, sometimes a type alias in tsoa. Simply called a referenceType, so that's the term for this I will use in the context of this explanation. This thing is not supported as QueryParam.

What I'm doing is adding a way to resolve ObjectLiteralTypes. That's what would allow you to not create a new interface every time you want to add an arbitrary amount of sub-properties to your property (of a referenceType) but to do it the TypeScript way of adding another set of angle brackets and specifying the sub-properties between. These types would then be inlined into the property schema of the referenceType

@HoldYourWaffle
Copy link
Contributor

Oh wait I think I get it, you're adding support for an "anonymous" object, just not in @Query.
That makes sense, thank you for making my dumb and tired brain understand.

As a side note, I might post a very work-in-progress PR with my implementation of refObject (and I suppose these literals too in the future) in @Query. I made this implementation 2 months ago in a really big rush (needed it for a project with a tight deadline) so it's nowhere near a "mergeable" state (I'm not even sure what some parts are doing anymore), but it might be good to get some feedback on the general direction I'm going.

@WoH WoH marked this pull request as ready for review August 24, 2019 13:58
@WoH
Copy link
Collaborator Author

WoH commented Aug 24, 2019

I'll open this one up, got some (admittedly lazy) JSDoc validation specs in.
Excess prop handling (only 'throw-on-extras' explicitly tested right now) should be covered in the koa no additional integration spec, but there's probably room for more.

@HoldYourWaffle
Copy link
Contributor

Just like with #444 we should consider which PR we want to merge first, my cleanup (#445) or this.
(Again assuming my cleanup gets merged)

@WoH
Copy link
Collaborator Author

WoH commented Aug 25, 2019

Anticipating some of @dgreene1's review in f3d9fb0 and 55f9ca7 to reduce the time he has to invest in PR review (hopefully).

I tried to improve UX for validations, throw if metadata is missing, added an assertion that a nested object value is an object in the first place.

Got around to spec excess prop handling (ignore, remove, throw), see 55f9ca7

Happy to get reviews at this point.

@@ -333,12 +422,6 @@ export class TestClassModel extends TestClassBaseModel {
stringProperty: string;
protected protectedStringProperty: string;

public static typeLiterals = {
Copy link
Collaborator Author

@WoH WoH Aug 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these since they were messing up type resolution (Boolean, Number and String will throw) now that we actually examine the content of the Block.

E: They were not actually used in tests anyway.

@WoH
Copy link
Collaborator Author

WoH commented Aug 30, 2019

Rebased for the green checkmark.

@WoH WoH force-pushed the deepobj branch 2 times, most recently from 3fe932a to d9a9367 Compare September 1, 2019 15:21
@WoH
Copy link
Collaborator Author

WoH commented Sep 1, 2019

@dgreene1 I just rebased and realized checks are not running anymore. Is this intended?

dgreene1 added a commit that referenced this pull request Sep 1, 2019
@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 1, 2019

@dgreene1 I just rebased and realized checks are not running anymore. Is this intended?

I don’t know what’s up with github actions. I just made a change that should hopefully force the tests to be run on each commit.

@WoH
Copy link
Collaborator Author

WoH commented Sep 1, 2019

I don’t know what’s up with github actions. I just made a change that should hopefully force the tests to be run on each commit.

Works now

PSA: f8eef44#diff-d107cbd1a26a9e3dbdf1db48ecdeac37R1 was apparently not subjected to the prettier hook, I had to do some trickery to get the automatic fix out of this PR, but I can append it if it's wanted, I did not, in order to avoid merge conflicts and keep this PR clean.

Copy link
Collaborator

@dgreene1 dgreene1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really fantastic job on this PR. I think those two places that were lacking intersection tests are the only places that I think are lacking coverage.

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.

3 participants