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

Update TypeDoc to latest - 0.24.6 #1093

Merged
merged 9 commits into from
May 5, 2023
Merged

Conversation

AndrewJakubowicz
Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz commented May 3, 2023

This bumps TypeDoc to the latest version. Only one minor patch from the last PR: #1090

My hope is that this gives us some more tools for the labs documentation. For example a new project root path option is now required & may fix the strange file paths seen in #1092.

Regardless - while I have some context in this area - I'm trying to get us as updated as possible.

Changes

  • TypeDoc removed kindString. We use this extensively in our api.html template so I needed to add it back using a full tree traversal.
  • TypeDoc now uses target to reference the id of the declaration for reflected nodes. This initially broke all linking between symbols. Fixed once I figured out the id was on target.
  • Small API compatibility changes.

Testing

Very manual process again.

This was tested by once again going page by page in the docs and diff checking the content.
I also checked the View Source links continued to work & generated pages look the same between prod and this PR.

Issues found and fixed list

These issues occurred because initially I did not fully traverse the TypeDoc tree to add kindString fields back. Issue resolved by a full traversal.

@github-actions
Copy link

github-actions bot commented May 3, 2023

A live preview of this PR will be available at the URL(s) below.
The latest URL will be appended to this comment on each push.
Each build takes ~5-10 minutes, and will 404 until finished.

https://pr1093-b007374---lit-dev-5ftespv5na-uc.a.run.app/
https://pr1093-a1f740f---lit-dev-5ftespv5na-uc.a.run.app/
https://pr1093-1ad52f3---lit-dev-5ftespv5na-uc.a.run.app/
https://pr1093-fd12058---lit-dev-5ftespv5na-uc.a.run.app/
https://pr1093-15047da---lit-dev-5ftespv5na-uc.a.run.app/
https://pr1093-c6f2a80---lit-dev-5ftespv5na-uc.a.run.app/

@AndrewJakubowicz AndrewJakubowicz marked this pull request as ready for review May 4, 2023 16:32
@AndrewJakubowicz
Copy link
Contributor Author

Manually tested and diff checked generated pages on the most recent preview URL and as far as I can find - we have no content changes.

External links (E.g., to MDN from native types work), and internal links, (e.g., ReactiveElement or PropertyDeclaration) are also working again.

@AndrewJakubowicz AndrewJakubowicz requested review from augustjk and justinfagnani and removed request for justinfagnani and augustjk May 4, 2023 16:39
This was referenced May 4, 2023
@AndrewJakubowicz
Copy link
Contributor Author

Thanks for the review - did a rebase with main. Will merge after reviewing new preview url.

Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

Ohh boy. Hoping we don't need to do another breaking typedoc update anytime soon.

Comment on lines 499 to +501
// The "target" key is unstable causing our "Check API data is in
// sync" approach to fail. We also do not use this key.
key === 'target'
key === 'target' ||
Copy link
Member

Choose a reason for hiding this comment

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

We're now using the key of 'target' for referencing reflected node as per the PR description, and usage in line 425. Do we now want to keep this around?

Copy link
Contributor Author

@AndrewJakubowicz AndrewJakubowicz May 5, 2023

Choose a reason for hiding this comment

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

Correct that we use target and id for tracking declarations and references to declarations. This just filters out keys that we don't need for pages.json -> the rendered html. So we can remove target from the cached output.

Edit: We use id and target to manipulate other data fields. E.g., location. But then they are not required as an artifact.

@@ -28,14 +28,14 @@ export interface MigrationComment

export interface ExtendedDeclarationReflection extends DeclarationReflection {
location?: Location;
externalLocation?: ExternalLocation;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add externalUrl here instead? Maybe not since we're just doing an inline assertion to add anyway, but also maybe yes for documentation purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

externalUrl is already documented natively on DeclarationReflection. E.g., it's a TypeDoc type that I am "correcting". However I can add it here for additional documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Oh if it's already there the no problem! I was just going based off the diff.

@AndrewJakubowicz
Copy link
Contributor Author

Haha! This is gonna be the last update for a while of this size. :P Thank you for the review.

@AndrewJakubowicz
Copy link
Contributor Author

🙏 Thank you so much!

@AndrewJakubowicz AndrewJakubowicz merged commit 5d73b61 into main May 5, 2023
@AndrewJakubowicz AndrewJakubowicz deleted the update-docs-typedoc-2 branch May 5, 2023 18:06
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