-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: add support for typescript with jsdoc #568
Conversation
- new eslint jsdoc plugin - add ts cmd with 'check', 'types', 'types-clean', 'docs' modes Modes: - `check` checks src and test folders for ts errors - `types` generates types declarations inline with the source files - `type-clean` deletes all the *.d.ts files - `docs` generates documentation based on the type declarations BREAKING CHANGE: The new jsdoc linter plugin can create new errors!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugomrdias can you sync this work with ChainSafe/js-libp2p-gossipsub#77? I would like to not go on different paths here.
cc @wemeetagain
That's a complete rewrite in ts and not what we want to do with the others repos. Check ipfs/js-ipfs#2945 |
What is the reasoning for aegir not supporting things like linting here? I think that we can get to a solution that fits both needs with this. Moreover, I think we should try to leverage the document generation feature that aegir is providing for typescript projects |
What do you mean ? linting specific for typescript ?
Do you mean the current one ? or the one in this PR using typedoc ? |
I mean that if
I mean this new feature in this PR. I don't want to move gossipsub with a a typescript decision now and later on decide that we want to make things differently to have the automated docs, if we are currently doing both work streams. So, I would like that we see if |
This PR adds support for typescript in the scope of this issue ipfs/js-ipfs#2945 not for full typescript projects. Mixing support for jsdoc type declarations and full typescript projects would be a giant mess. To implement full ts support we would need another command and another eslint config that would be selected in the lint cmd. |
Got it. I am still wondering if we should go on the same way on |
|
||
const EPILOG = ` | ||
Presets: | ||
\`check\` Checks src and test folders for Typescript errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this mean we have to include TS comments on all of our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it works as is but it may output errors for some inferred types or defined types in the source code.
Checking the tests enables us to be the users of our own types and be able to identify issues in the types early.
@@ -112,7 +97,7 @@ exports.getPathToNodeModules = () => { | |||
/** | |||
* Get the config for Listr. | |||
* | |||
* @returns {Object} | |||
* @returns {object} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has Object
changed to object
? Is Object
not the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the linter default https://github.com/gajus/eslint-plugin-jsdoc#why-not-capital-case-everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions inline..
What is left to do on this PR? |
Nothing its good, just needs to be integrated with the others cmds, by itself its done.
|
} | ||
|
||
const check = async (forwardOptions) => { | ||
const configPath = fromRoot('tsconfig-check.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to fail if there is no tsconfig.json
(which can include single extends
directive e.g. { "extends": "aegir/src/config/aegir-tsconfig.json"}
) than do this. Otherwise we introduce another instance of other tools like vscode making different assumptions than tools than aegir.
@achingbrain @hugomrdias can I land this, so I could integrate it into ipfs/js-ipfs#3281 ? I think I would want to make some additional changes too, like the ones @Xmader has suggested, because I did run into all those in the mentioned pull. @hugomrdias should I just take over this, or would you rather have me do another pull that builds upon this ? |
@Gozala just take over this PR or make a new whatever is best to land easy and stable support to ts jsdocs. You already know my main concerns around this topic so just go for it. |
Co-authored-by: Xmader <xmader@outlook.com>
So this pull request does following things
I'll do the the 2nd under #637 and split this up into separate pulls for 3 and 4. |
Changes
Modes:
check
checks src and test folders for ts errorstypes
generates types declarations inline with the source filestypes-clean
deletes all the *.d.ts filesdocs
generates documentation based on the type declarationsBREAKING CHANGE: The new jsdoc linter plugin can create new errors!
If we go we this, we still need to swap current documentation with this and add types generation to release cmd.
This PR does not support full typescript projects! This is only for javascript with jsdoc generated types declarations.