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

Enable TS docs generation. #1324

Merged

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Jan 6, 2022

With the TS additions, the docs generator was failing the build. This PR prepares the groundwork for generating docs from TS/TSX source files.

Additional work is required to properly generate the content

  • in TSX files, the prop types are not properly generated some values always result in undefined
  • in TS files I had to use tsdoc instead of JSdoc so the content is not generated at all.

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #1324 (39259f0) into master (ff198cb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1324   +/-   ##
=======================================
  Coverage   65.75%   65.75%           
=======================================
  Files         233      233           
  Lines        4068     4068           
  Branches     1458     1458           
=======================================
  Hits         2675     2675           
  Misses       1234     1234           
  Partials      159      159           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff198cb...39259f0. Read the comment docs.

@Hyperkid123 Hyperkid123 marked this pull request as ready for review January 6, 2022 14:29
@Hyperkid123 Hyperkid123 requested a review from a team January 6, 2022 14:29
Copy link
Collaborator

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Looking good! Just two small questions.

const files = glob.sync(`${componentsSrc}/**/*.js`).filter(file => !file.match(/((test|spec|index).js|(\/__mock__\/|\/__mocks__\/|\/test\/))/gmi));
const files =
glob
.sync(`${componentsSrc}/**/*.{js,ts,tsx}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about excluding d.ts?

Suggested change
.sync(`${componentsSrc}/**/*.{js,ts,tsx}`)
.sync(`${componentsSrc}/**/*.[!d.]{js,ts,tsx}`)

Not sure about the dot, if it needs to be escaped or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karelhala the .d.ts files are filtered on the line below. The glob pattern does not play very nicely with eclusions. I find it simpler to read to split the exclusions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mhh it feels a bit too heavy to call filter just to remove .d.ts files if we could just add ?!(d.) to it (previously the ignore was not correct).

That being said if you'd still prefer the filter to do the job I'd rather join them together. This way we are perfomring one extra loop just to ignore them, whic honestly feels a bit too complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karelhala I don't think glob supports ?!(d.). That is why I've gone with the additional filter.


async function parseFile(file) {
async function parseTSFile(file) {
return new Promise((resolve, reject) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it's the same pattern as previously used. But I am not a fan of callbacks, could this be solved by using util.html#utilpromisifyoriginal? That way we'd fully use async/await and no need to mess with callbacks and nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean it could, but then this one function would be entirely different to every other user to generate the docs.

I can imagine this to be a nice task for one of our upcoming interns to get familiar with the environment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point! I've created issue to addess this #1326

@Hyperkid123 Hyperkid123 merged commit 6a5d9c1 into RedHatInsights:master Jan 10, 2022
@Hyperkid123 Hyperkid123 deleted the enable-ts-docs-generation branch January 10, 2022 14:13
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