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

Have tsd-jsdoc generate default exports #98

Merged
merged 43 commits into from
May 2, 2020

Conversation

alxroyer
Copy link
Contributor

@alxroyer alxroyer commented Sep 2, 2019

I'm starting a draft PR for issue#96.

Still work to be done on it, but I share the code right now so that one could review it and give me advice on how to lead the things.

Still buggy: module declaration is generated twice.
…t' doclets.

=> fixed: generation of 'export default' at the end of the module (and not at the beginning)
=> fixed: duplicate module declaration
src/Emitter.ts Outdated Show resolved Hide resolved
alxroyer and others added 7 commits September 3, 2019 17:11
`debug()` controled by a specific `debug` option.
'documented' => by default
'exported' => not fully implemented, just prevents non documented doclets to be removed for the moment
@alxroyer
Copy link
Contributor Author

alxroyer commented Oct 3, 2019

Status on this PR.

At this time, all tests pass successfully, either in 'documented' or 'exported' generation strategies.

Brings improvements and fixes, in particular for the following:

What remains to test/do:

  • issue#96: Default exports not working
    • To be tested with real code
    • As exeprimented lately, module.exports= in .js does not fit with export default in .d.ts (see stackoverflow.com and TypeScript#2242)
  • issue#100: Generation strategy : what is documented vs what is actually exported
    • To be tested with real code

@englercj
Copy link
Owner

englercj commented Jan 5, 2020

@Alexis-ROYER, Have you had a chance to revisit this now that the holidays are passing behind us?

@alxroyer
Copy link
Contributor Author

alxroyer commented Jan 8, 2020

Hi @englercj, sure, it has been a while since the last status...

Here's an update:

  • issue#96: I've had the opportunity to test the things further around default exports: TypeScript syntax & transpilation options, .tsd syntax, node options... I plan to publish a repository that clarifies the way it works, with the recommendations I concluded from my tests.
  • Now, to my point point of view, the really most important thing remaining is to test it with real code and check it is valid enough.

This is on the way, but as I've been busy on other things, I must admit it has been delayed. I let you known as soon as I get back to it.

@alxroyer
Copy link
Contributor Author

I eventually managed to get it done.

Just for the story: while integrating walk-back (commit bf2eaab, Oct 2019) I could not get a .d.ts file working with an ES6 default import / export, which led me to doubts on the way default exports should work.

That's the reason why I went through a dedicated study (see tsd-default-export). This study brought to formulate recommendations on default exports, which recommendations I have applied in commit 1ac9c2c, Apr 2020.

Status on this PR:

  • On my side, all tests pass successfully:
    • Every test is executed twice: first with the documented generation strategy, then with the exported generation strategy
    • Addition of 32 tests for default exports
    • Addition of constructor_all.js test
  • Brings improvements and fixes, in particular for the following:

Note on the exported generation strategy: requires a @module tag in the .js input file.

Ready for review.

@alxroyer alxroyer marked this pull request as ready for review April 20, 2020 09:17
@englercj
Copy link
Owner

Awesome! Thanks for all the hard work @Alexis-ROYER. I'll take a look at this today.

Copy link
Owner

@englercj englercj left a comment

Choose a reason for hiding this comment

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

Wow, thanks again for this PR @Alexis-ROYER. This is a great change.

All my comments were stylistic, and I tried to add suggestions that can be added in directly for most of them. Thanks again!

src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
src/Emitter.ts Outdated Show resolved Hide resolved
'exported' tests were not executed due to a false trailing 'r' after `generationStrategy` in `test/lib/index.ts`.
@alxroyer alxroyer requested a review from englercj April 26, 2020 00:00
@alxroyer
Copy link
Contributor Author

alxroyer commented May 1, 2020

@englercj I think I managed to fix all the points so far. Let me know if you have any more questions or remarks.

@englercj englercj merged commit d759376 into englercj:master May 2, 2020
@englercj
Copy link
Owner

englercj commented May 2, 2020

This was awesome @Alexis-ROYER, thanks again for all your hard work!

@alxroyer
Copy link
Contributor Author

alxroyer commented May 2, 2020

My pleasure.

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