search

otris / jsdoc-tsd

JSDoc template for generating TypeScript definition files based on JSDoc comments.
MIT License
29 stars 7 forks source link

Is @extends support working properly? #56

Closed whitlockjc closed 5 years ago

whitlockjc commented 5 years ago

Whenever I use class B extends A in JavaScript, with the appropriate @extends tag, instead of the TSD containing class B extends A, I end up with all members of A being duplicated in B. Is this the desired approach?

wehrstedt commented 5 years ago

I was able to reproduce this. No, its not the desired approach. I added a test on my branch to reproduce this temporay (see here).

MichalCz commented 5 years ago

@wehrstedt it seems now working in scramjet... I'll try to remove some of my overrides.

MichalCz commented 5 years ago

...and confirmed - extends DataStream are now nicely added without my awful hack. :)

wehrstedt commented 5 years ago

@MichalCz No duplicated members?

MichalCz commented 5 years ago

I need to check if I don't have any counter measures for this. I'll get back to you in a couple minutes.

MichalCz commented 5 years ago

Yup. I managed to remove this much of my hacks: see commit.

Seems to work well and doesn't add the derived methods.

wehrstedt commented 5 years ago

Strange, because I can repruduce this error with a simple example as shown here: https://github.com/wehrstedt/jsdoc-tsd/commit/68d6773689c91e4549cb808fd9eba694e7de5c9a

MichalCz commented 5 years ago

I may have some filtering in place in another part of my plugin.

I'll check tomorrow and report back.

MichalCz commented 5 years ago

Oh, ok, now I noticed... Yes, I still have those extends replacements...

MichalCz commented 5 years ago

So I was trying to get around the code and wrote a bit of code that would look up the right extends and set it. This would go around the same place where members are assigned to top-level objects, but the thing is that since StringStream extends DataStream and since DataStream is @memberof module:scramjet it can't look it up properly.

I'll set up a fork tomorrow and push my changes as work in progress. Maybe it'd make more sense to you - I sadly could continue no sooner than Monday evening.

MichalCz commented 5 years ago

I pushed changes here: https://github.com/otris/jsdoc-tsd/compare/master...MichalCz:feature/class-extends?expand=1

MichalCz commented 5 years ago

@wehrstedt I pushed a commit that resolves classes. Please take a look in the link above.

wehrstedt commented 5 years ago

Can you have a look at this test case? https://github.com/wehrstedt/jsdoc-tsd/blob/issue/56/test/class/test.parseClass.ts#L161

Is this the setup in your project? I moved StringStream to a separate namespace to avoid having all classes in the same module.

No also the classes will be duplicated. Strange...

MichalCz commented 5 years ago

It's a bit different: StringStream is also @memberof module:scramjet and extends DataStream.

Best seen here in NumberStream extends DataStream

wehrstedt commented 5 years ago

@MichalCz I found the issue.

  1. I fixed the duplication of members and classes with 34f970de04bf7a24687d099ec670abd53e02fa45 and cc997ac0db72d5ddccd44ee02d7164fc39c90d9c (the second issue only occurres in tests because of calling resolveMembership multiple times).
  2. I cherry-picked your changes because after my fixes there were some merge conflicts.
  3. The issue that classes of modules currently can't be resolved is because of the naming schema of jsdoc. Take a look at the test I added with 2a1a95bbda9e810f5545014c9c93b56003709b63. The extends relation is written as module:scramjet~DataStream (note the ~). The longname of this item is module:scramjet.DataStream (now it's written with a dot).

Thats why the comparison will not succeed because you are comparing the ~ version with the dotted version. Addtionally this comparison will not become true because cls.original.name equals the name of the class (DataStream, not the longname) and augments equals the longname of the class (module:scramjet~DataStream).

I had a similar problem solved with 1b8ddd96563e5b31ea74935c15bfd2124078c200. You can easily debug this by starting this test

wehrstedt commented 5 years ago

I forgot to mention that your changes for extending classes works, good job. I also added a test with 4a58cde526e9ffb1059b31e17851c840ad9c0d79 for that, thank you.

MichalCz commented 5 years ago

Cool and no probs. :)

wehrstedt commented 5 years ago

So I was trying to get around the code and wrote a bit of code that would look up the right extends and set it. This would go around the same place where members are assigned to top-level objects, but the thing is that since StringStream extends DataStream and since DataStream is @memberof module:scramjet it can't look it up properly.

I'll set up a fork tomorrow and push my changes as work in progress. Maybe it'd make more sense to you - I sadly could continue no sooner than Monday evening.

@MichalCz Do you want to fix the lookup for modules? Its the only missing part to finish this PR.

MichalCz commented 5 years ago

Im sorry, but I have so much extra work that I can't reliably commit to any time spent there. I'll create a separate PR for this later

wehrstedt commented 5 years ago

@MichalCz I applied the changes you and I have already done with #74. I will create a separate issue for the support of module members