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

feat: break up api rendering #611

Merged
merged 9 commits into from
Nov 9, 2021
Merged

feat: break up api rendering #611

merged 9 commits into from
Nov 9, 2021

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Nov 5, 2021

Break up the rendering of a packages documentation into multiple pages.
The root package page now renders the readme, and each type within the
packages API reference is rendered on its own page. Items within the
readme and types are still linkable via hash links that are scrolled to
by the PackageDocs component.

The rendering is broken up by splitting the incoming markdown on the
API Reference header and then on subsequent headers within the api
reference markdown string. These are then parsed into menu items and a
hash map of typeid -> type markdown content.

Adds unit tests for markdown parsing logic to ensure outputs remain
stable.

Removes 1MB size limit for rendering markdown documents.

NOTE: setext style md
headings are not currently supported and will be added in a follow up. See
#629

fix: #560

Break up the rendering of a packages documentation into multiple pages.
The root package page now renders the readme, and each type within the
packages API reference is rendered on its own page. Items within the
readme and types are still linkable via hash links that are scrolled to
by the PackageDocs component.

The rendering is broken up by splitting the incoming markdown on the
`API Reference` header and then on subsequent headers within the api
reference markdown string. These are then parsed into menu items and a
hash map of `typeid -> type markdown content`.

Adds unit tests for markdown parsing logic to ensure outputs remain
stable.

fix: #560
@MrArnoldPalmer MrArnoldPalmer requested a review from a team November 5, 2021 21:07
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Yeah!

Curious- did you have a chance to benchmark the load time for large modules against the previous version?

@addihorowitz
Copy link
Contributor

addihorowitz commented Nov 7, 2021

Yeah!

Curious- did you have a chance to benchmark the load time for large modules against the previous version?

@MrArnoldPalmer It should be easy to adapt the performance test script to test and compare the performance before and after. LMK if you'd

@MrArnoldPalmer
Copy link
Contributor Author

Yeah!

Curious- did you have a chance to benchmark the load time for large modules against the previous version?

Only with local builds, which is not a super accurate number because of dev assets instead of our optimized product assets. Though the difference was clear and that should stand.

I can run some comparisons in my account and report back.

@MrArnoldPalmer
Copy link
Contributor Author

@eladb @addihorowitz here are numbers comparing the ec2 package rendering performance before and after this PR.

Root documentation page rendering time with loading assets:
New: approx 1.5 seconds
ec2optimized
Old: approx 14.5 seconds
ec2old

These screenshots are the browser tools timeline showing the first frames that include rendered markdown content of the readme.

Here are also the lighthouse performance reports that you can view in your browsers dev tools for more detailed information. You can see that though performance is obviously greatly improved, the performance score in fact goes DOWN. This is (as far as we know) because the old version of the site takes so long to render that lighthouse and pagespeedinsights actually assume that it is done much earlier than in reality, and show first contentful paint around 2 seconds, and time to interactive on desktop around 5, though we know this isn't true.

ec2reports.zip

@MrArnoldPalmer MrArnoldPalmer requested a review from eladb November 8, 2021 18:00
@eladb
Copy link
Contributor

eladb commented Nov 8, 2021

Hell yeah! Looks like amazing improvement

src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageTypeDocs.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
src/views/Package/PackageState.tsx Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 9e7d926 into main Nov 9, 2021
@mergify mergify bot deleted the feat/separate-type-pages branch November 9, 2021 20:35
mergify bot pushed a commit to cdklabs/construct-hub that referenced this pull request Nov 18, 2021
The type links construct-hub creates are broken because they still format links using the old `#` pattern. 

We dropped this in cdklabs/construct-hub-webapp#611 but didn't adjust the link formatter we pass to docgen to account for it.

This video shows the experience with links now. Note that clicking on a type link now reloads the page, which is worse experience that we used to have, and what we currently have with readme links. 

https://user-images.githubusercontent.com/1428812/142417380-d4045665-c775-4ae6-a491-3bce2381743a.mov

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

Separate types into pages
5 participants