-
Notifications
You must be signed in to change notification settings - Fork 821
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
chore: fix cross project links and missing implicitly exported types #3533
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3533 +/- ##
=======================================
Coverage 93.80% 93.80%
=======================================
Files 249 249
Lines 7640 7640
Branches 1589 1589
=======================================
Hits 7167 7167
Misses 473 473 |
The |
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.
Thank you for taking care of this 🙂
Should the MetricsAPI
type be exported directly from the API package? Right now it looks like it is missing. 🤔
I ran the docs step locally and I think the file that's making the lint step fail will end up at
https://open-telemetry.github.io/opentelemetry-js/docs/classes/_opentelemetry_api._internal_.MetricsAPI.html
instead of https://open-telemetry.github.io/opentelemetry-js/classes/_opentelemetry_api.MetricsAPI.html
.
So the lint step may continue to fail. 🤔
@@ -62,7 +62,9 @@ | |||
"markdownlint-cli": "0.32.2", | |||
"prettier": "2.8.0", | |||
"semver": "7.3.5", | |||
"typedoc": "0.22.10", | |||
"typedoc": "0.22.18", | |||
"typedoc-plugin-missing-exports": "1.0.0", |
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.
Doesn't this add internal details to our docs? Is that wanted? What is it fixing?
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 the underlying problem may be that we're not exporting MetricsAPI
from the API package - that makes the lint step fail as it is not part of the docs. 🤔
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.
Since this was never a problem with the other APIs, there is something being done differently then with metrics that needs to be fixed. I think this plugin is hiding a problem rather than fixing it.
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.
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.
Some types are not exported but rather their instances are, like in https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/src/detectors/BrowserDetector.ts#L64. It is fine to use those values without their types exported. However, typedoc would explain these and would not document their types and link to their base classes.
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.
Looks good; thanks for the explanation in https://github.com/open-telemetry/opentelemetry-js/pull/3533/files#r1069636994 🙂
Which problem is this PR solving?
Fixes
docs:test
failure in PRs like #3514 and #3517.Short description of the changes
typedoc-plugin-missing-exports
to generate docs for implicitly exported types.typedoc-plugin-resolve-crossmodule-references
to generate links between projects in the monorepo.Type of change
How Has This Been Tested?
Tested locally with
NODE_OPTIONS=--max-old-space-size=6144 npm run docs
and verified that the docs are generated as expected.Checklist: