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.

Sign up for GitHub

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

Jump to bottom

What support for @override tags? #104

Open
alxroyer opened this issue Sep 18, 2019 · 7 comments
Open

What support for @override tags? #104

alxroyer opened this issue Sep 18, 2019 · 7 comments
Labels
bug

Comments

@alxroyer
Copy link
Contributor

I'm opening this issue in order to clarify what should be the appropriate behavior for @override tags.

As I'm working on making the tests pass on PR#98, I'm a bit confused with the output for the MyThing.copy() override in test/fixtures/class_all.js (see my commit be91778).

According to PR#11, I understand that we should prioritize the parent's overridden doclet over the downclass's overriding one (which sounds weird to me).

But according to PR#103, it seems that the port to jsdoc@3.6.3 led us to prioritize the downclass's overriding doclet over the parent's overridden one (which seems more logical to me).
Indeed, jsdoc@3.6.3 raw output (jsdoc -X) differs from jsdoc@3.5.5, particularly with override doclets, especially with the final module:util~MyThing#copy doclets that receive the first module:util~MyThing#copy doclet in jsdoc@3.6.3 while it received the OtherThing#copy doclet in jsdoc@3.5.5.

If I get back to jsdoc documentation for @override tags, I understand that @override tags could be considered as optional, in as much as jsdoc automatically detects overrides without the help of @override.
Nevertheless, when we remove the @override tag in test/fixtures/class_all.js, the raw jsdoc output is different:

  • jsdoc@3.5.5:
    • The first module:util~MyThing#copy doclet:
      • no longer has a "override": true attribute,
      • no longer has a "ignore": true attribute,
      • but receives a "overrides": "OtherThing#copy" attribute.
    • The final module:util~MyThing#copy doclet is not generated anymore.
  • jsdoc@3.6.6:
    • Same behavior for the first module:util~MyThing#copy doclet.
    • The final module:util~MyThing#copy docletS (two in a row) are not generated anymore.

On the typsecript side, it seems that in the PR#103 version of the test/expected/class_all.d.ts file, typescripts complains with the following error on MyThing.copy():

Property 'copy' in type 'MyThing' is not assignable to the same property in base type 'OtherThing'.
  Type '(other: OtherThing) => void' is not assignable to type '() => void'.

So, even though the PR#103 way looked more logical, it seems that it eventually leads to invalid typescript.

In the end, I have the feeling that 'explicitly overridden members (i.e. a method that has a different signature)' as named in PR#11 should not be handled.
If so, the @override test case in test/fixtures/class_all.js could be removed.
@englercj
Copy link
Owner

@override is listed in the readme as an unsupported tag because there is no way we can declare a function of the same name, but different signature in a derived class in typescript currently.

As such (IIRC, because it was years ago) I was just skipping over anything that was inherited to try and generate valid TS. It wasn't that I was preferring the base class one, so much as I just couldn't define the child class version without generating invalid TS.

If the signatures are the same, you don't need to define it in the child because it is inherited. If the signatures are different, that is invalid TS. So I just dropped it.

I don't have strong feelings about how this should work. I think always generating the method, which might be invalid TS, is also OK. At least then the user knows they did something wrong.

@alxroyer
Copy link
Contributor Author

Thank you very much for the clarification @englercj.

Then could we remove the @override tag from the test/fixtures/class_all.js test and align the signatures of the copy() methods in order to stay on the test case we actually support?

@englercj
Copy link
Owner

englercj commented Sep 19, 2019 •
edited
Loading

I'd like to have some test for the behavior of @override specifically to find where it breaks this lib.

While we can't support the actual meaning of @override, it would be nice if it behaved in a well-defined way and we had a test to catch regressions.

@alxroyer
Copy link
Contributor Author

Shall we say that, as long as TS does not support different signatures of overriding methods, the tsd-jsdoc should not generate types for overriding methods, whether these overriding methods have a different signature or not from the overridden ones?
In addition, we could display a warning when a @override tag is encountered.
Let me know.

@englercj
Copy link
Owner

Yes I think that is what the previous behavior was. If you inherit a method of the same name as one you have defined, we drop the child one.

I don't feel strongly about the override warning, I'm OK either way.

alxroyer added a commit to alxroyer/tsd-jsdoc that referenced this issue Sep 20, 2019
@alxroyer
Warning addition while waiting for [tsd-jsdoc#104](englercj#104 resol…
390222c 
…ution.
@alxroyer
Copy link
Contributor Author

Thank you very much, that will help me for PR#98.

@alxroyer
Copy link
Contributor Author

alxroyer commented Oct 3, 2019

Fix proposal in PR#98 with commit 8b205c65.

@alxroyer alxroyer mentioned this issue Oct 3, 2019
Merged
@englercj englercj added the bug label Jan 17, 2020
englercj pushed a commit that referenced this issue May 2, 2020
@alxroyer @HackbrettXXX
Have tsd-jsdoc generate default exports (#98)
d759376 
* Very first 'export default' generation.
Still buggy: module declaration is generated twice.

* Removal of the IExportDefaultDoclet hack.

* Code moved from _createTreeNodes() to _buildTree() for 'export default' doclets.
=> fixed: generation of 'export default' at the end of the module (and not at the beginning)
=> fixed: duplicate module declaration

* 'module.exports =' pattern management.
module2 test addition.

* module3 test addition.

* Removal of K&R style braces.
`debug()` controled by a specific `debug` option.

* Addition of debug traces.

* opts.generationStrategy option addition.
'documented' => by default
'exported' => not fully implemented, just prevents non documented doclets to be removed for the moment

* correctly write doclets at es6 class constructors
cherry-pick & merge from @HackbrettXXX's commit 312578e

* add a test case for constructors (fails currently)

* Constructor generation with documentation.
From [PR#74](#74) with fix.

* Emitter._buildTree() strengthening with full jsdoc doclets.

* First 'exported' generation strategy implementation. Still needs to be tested.

* Test additions for testing the 'exported' generation strategy.
Lots of them are not enabled yet and need to be fixed.

* (Re)named exports.

* Warning addition while waiting for [tsd-jsdoc#104](#104 resolution.

* 'export <named type>' pattern support.
Fix for 'module.exports = {name: }' pattern.

* Named exports with reference to a type of the same name.

* 'export default <named type>' pattern support (works like a lambda class would actually).

* 'export default <lambda class>' pattern support.

* 'export default <lambda function>' and 'export default <named function>' patterns support.

* 'module.exports=<lambda type>' and 'module.exports=<named type>' patterns support.

* 'module.exports.name=<lambda type>' and 'module.exports.name=<named type>' patterns support.

* 'exports.name=<lambda type>' and 'exports.name=<named type>' patterns support.

* 'module.exports={name=<lambda type>} and 'module.exports={name=<named type>}' patterns support.
Workaround needed for classes as long as issue [jsdoc#1699](jsdoc/jsdoc#1699) is not fixed.

* cyclic dependencies with 'exported' generation strategy.
Addition of 'export default {name: ...}' tests (not supported yet).

* Test class_all.js working with 'exported' generation strategy.

* Test constructors.js & enum_all.js working with 'exported' generation strategy.

* Tests function_all.js and interface_all.js working with 'exported' generation strategy.
Warn&debug improvements.

* Test namespace_all.js working with 'exported' generation strategy.

* Tests property_all.js and typedef_all.js working with 'exported' generation strategy.

* 'documented' & 'exported' test cases.
Instrumentation for jsdoc 3.5.5 vs 3.6.3 investigations.

* jsdoc@3.6.3 installation.

* jsdoc@3.6.3 fixes.

* `walk-back.d.ts` typescript declaration.

* `walk-back.d.ts`: application of "tsd-default-export" recommendations.
See: https://github.com/Alexis-ROYER/tsd-default-export/blob/master/README.md

* Corrections to @englercj’s remarks.

* Fix merge tag 'v2.5.0' into export-default
'exported' tests were not executed due to a false trailing 'r' after `generationStrategy` in `test/lib/index.ts`.

* Corrections to @englercj’s remarks.

Co-authored-by: Lukas Hollaender <lukas.hollaender@yworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@englercj @alxroyer