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

chore: Test Docusaurus webpack 5 support #616

Closed
wants to merge 10 commits into from

Conversation

slorber
Copy link

@slorber slorber commented Apr 26, 2021

Use an unreleased version of Docusaurus to test the webpack5 support works for this site.

See also facebook/docusaurus#4089

It might improve performances on rebuild, but it requires that you set up caching of node_modules/.cache across your CI builds (not sure how this site is built exactly)

Running locally:

  • First build: 1700s
  • Second build: 350s

@welcome
Copy link

welcome bot commented Apr 26, 2021

Welcome! Thank you and congrats on your first pull request.

What happens next?

  • CircleCI is building a preview of the site with your modifications.
  • Once the build is complete a preview will be available and a comment with a link will be posted.
  • A member of the team will then review the pull request and provide feedback as needed.
  • If all is good and the pull request has passed review, you will be able to merge the PR.

Good luck to us all!

@CLAassistant
Copy link

CLAassistant commented Apr 26, 2021

CLA assistant check
All committers have signed the CLA.

@slorber slorber marked this pull request as draft April 26, 2021 07:56
@guardrails
Copy link

guardrails bot commented Apr 26, 2021

⚠️ We detected 1 security issue in this pull request:

Vulnerable Libraries (1)
Severity Details
High [email protected] - no patch available

More info on how to fix Vulnerable Libraries in JavaScript.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@sserrata
Copy link
Contributor

Hi @slorber, thanks for helping to test the performance benefits of webpack 5 with our site!

FYI, @glicht discovered we could shave ~12 minutes off the server build time by wrapping the DocSidebar component in BrowserOnly. See https://github.com/demisto/content-docs/blob/master/src/theme/DocSidebar/index.js#L7 for details. It's a small tweak with considerable benefits for large sites.

@sserrata
Copy link
Contributor

Concerning ValidationError: "custom_edit_url" is not allowed to be empty, I believe we are using empty strings ("") to hide the edit link on some docs. Could that be what's causing this error? If so, is there another way we can disable the edit URL/link?

@slorber
Copy link
Author

slorber commented Apr 26, 2021

Hi @slorber, thanks for helping to test the performance benefits of webpack 5 with our site!

NP, I'd be happy to help improve the perf of this site as it's probably one of the largest Docusaurus site and a good benchmark showing that we should rather improve 😅 !

FYI, @glicht discovered we could shave ~12 minutes off the server build time by wrapping the DocSidebar component in BrowserOnly. See https://github.com/demisto/content-docs/blob/master/src/theme/DocSidebar/index.js#L7 for details. It's a small tweak with considerable benefits for large sites.

Thanks, I think it makes sense that Docusaurus could only render collapsed sidebar items on the client, after hydration already happened. Didn't think the sidebar could have a significant impact on build time, but it makes sense for very large sidebars like yours. Will note to investigate that, but I'm not sure it's a good idea as it can have impacts on SEO, as crawlers could have a harder time finding links of your site.

ValidationError: "custom_edit_url" is not allowed to be empty

This is a new frontmatter validation we added recently, but will relax it to make it allow empty strings as before.


BTW, unrelated but can you add your apps to the Docusaurus showcase, please?
https://docusaurus.io/showcase

@glicht
Copy link
Contributor

glicht commented Apr 26, 2021

@slorber we can add caching of node_modules/.cache but it will be a cache that is refreshed based on a time period. For example refresh the cache once every X weeks. I think it should provide value as most of our pages don't change too often.

@glicht
Copy link
Contributor

glicht commented Apr 26, 2021

@slorber also wanted to say thanks on this effort. Really appreciated.

I am looking at the build log and I see that the build is failing on an empty value for custom_edit_url. I think this is a change in how docusaurus treats this front matter property.

@slorber
Copy link
Author

slorber commented Apr 27, 2021

@slorber we can add caching of node_modules/.cache but it will be a cache that is refreshed based on a time period. For example refresh the cache once every X weeks. I think it should provide value as most of our pages don't change too often.

I don't think it's a problem if once every X weeks you have a full rebuild with a cold cache. You'll only get the speed improvements if the cache is persisted, which is still better than the current state.

@slorber also wanted to say thanks on this effort. Really appreciated.

no problem :) helping you will help the whole Docusaurus community in the end

I am looking at the build log and I see that the build is failing on an empty value for custom_edit_url. I think this is a change in how docusaurus treats this front matter property.

Yes definitively, I'll fix this and update the PR. Was able to build by doing some local doc changes.

Fixed in facebook/docusaurus#4687

@slorber
Copy link
Author

slorber commented Apr 27, 2021

Noticed on your doc that you have both a markdown title + a frontmatter title:

---
id: 19.10.0
title: "19.10.0"
custom_edit_url: https://github.com/demisto/content-docs/blob/master/content-repo/extra-docs/releases/19.10.0.md
hide_title: true
---

# Demisto Content Release Notes for version 19.10.0 (30654)
##### Published on 02 October 2019
### Integrations

Can you explain the motivation?

My intuition is that you want the page title to be the frontmatter.title, and the markdown title to be the user-visible h1 heading. I'm restoring this behavior that changed recently in facebook/docusaurus#4665 (comment)
But I find this behavior a bit implicit and not really defined/documented, so if you can explain me your use-case + ideal API to achieve it I'd be happy to ear it.

Also, there's the hide_title: true being used, probably to avoid a duplicate h1 title issues we previously had.
Since alpha 73 you should remove it because it will prevent any heading (from BOTH md or frontmatter) to appear at the top of your doc, so it will really hide the title:

image

slorber added 3 commits April 27, 2021 16:57
# Conflicts:
#	docusaurus.config.js
#	package-lock.json
#	package.json
# Conflicts:
#	docusaurus.config.js
#	package-lock.json
#	package.json
@slorber
Copy link
Author

slorber commented Apr 27, 2021

Upgraded the PR to latest repo changes but had some issues:

  • index.json for marketplace is missing (not sure how to generate it)
  • query-string package is not in package.json

Also suggesting to remove the hide_title frontmatter as the release pages don't have titles anymore:

image

It's probably worth it to "re-swizzle" your custom theme comps at some point to upgrade them, as some code has changed and you might want to sync with our upstream changes

@sserrata
Copy link
Contributor

Upgraded the PR to latest repo changes but had some issues:

  • index.json for marketplace is missing (not sure how to generate it)
  • query-string package is not in package.json

The command npm run marketplace-docs will generate the index.json needed for marketplace. As for query-string, it appears to work even without it being explicitly installed/included in package.json. Could it be built-in?

@slorber
Copy link
Author

slorber commented Apr 30, 2021

As for query-string, it appears to work even without it being explicitly installed/included in package.json. Could it be built-in?

Not sure why it does not work only for me, but any deps you use directly should be in package.json, so I added it

@slorber
Copy link
Author

slorber commented Apr 30, 2021

Can you please approve the CI workflow. This is a new thing required now, to protect against hackers using CI to farm bitcoins 😅

@slorber slorber marked this pull request as ready for review April 30, 2021 17:49
@slorber
Copy link
Author

slorber commented May 1, 2021

fixed a little breaking change in Docusaurus code in the marketplace sidebar code.

this PR is ready for review now

@Itay4
Copy link
Contributor

Itay4 commented May 1, 2021

Can you please approve the CI workflow. This is a new thing required now, to protect against hackers using CI to farm bitcoins 😅

approved

@github-actions
Copy link

github-actions bot commented May 1, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@github-actions
Copy link

github-actions bot commented May 1, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@slorber
Copy link
Author

slorber commented May 4, 2021

Hi

Please let me know if you find any issues with this PR, as I'm trying to see if the released Webpack 5 support is good ;)

I suggest to remove the hide_title: true frontmatter from your release notes as it really hides the doc title now 🤪

@glicht
Copy link
Contributor

glicht commented May 4, 2021

Noticed on your doc that you have both a markdown title + a frontmatter title:

---
id: 19.10.0
title: "19.10.0"
custom_edit_url: https://github.com/demisto/content-docs/blob/master/content-repo/extra-docs/releases/19.10.0.md
hide_title: true
---

# Demisto Content Release Notes for version 19.10.0 (30654)
##### Published on 02 October 2019
### Integrations

Can you explain the motivation?

My intuition is that you want the page title to be the frontmatter.title, and the markdown title to be the user-visible h1 heading. I'm restoring this behavior that changed recently in facebook/docusaurus#4665 (comment)
But I find this behavior a bit implicit and not really defined/documented, so if you can explain me your use-case + ideal API to achieve it I'd be happy to ear it.

Also, there's the hide_title: true being used, probably to avoid a duplicate h1 title issues we previously had.
Since alpha 73 you should remove it because it will prevent any heading (from BOTH md or frontmatter) to appear at the top of your doc, so it will really hide the title:

image

Hi @slorber sorry for the late response. Our motivation is basically to have a different h1 title on the page than the one used in the sidebar. See the example image:

image

Will this be possible with the upcoming Docusaurus update?

@slorber
Copy link
Author

slorber commented May 6, 2021

@glicht yes we actually have a better frontmatter for that usecase:

---
id: 19.11.0
- title: "19.11.0"
+ sidebar_label: "19.11.0"
custom_edit_url: https://github.com/demisto/content-docs/blob/master/content-repo/extra-docs/releases/19.11.0.md
- hide_title: true
---

# Demisto Content Release Notes for version 19.11.0 (33434)

As it's on a separate repo, I can't include this in the current PR and the preview will remain broken: https://pull-request-616--demisto-content-docs.netlify.app/docs/reference/releases/21.4.1

By the way, the config now accept an edit URL function, this may be helpful to replace your hardcoded custom_edit_url

@glicht
Copy link
Contributor

glicht commented May 7, 2021

@slorber thanks for the clarification. When we go and update we will change the frontmatter.

I am going to try to also enable the caching on this PR to see the effect. Will be committing directly to your branch.

@slorber
Copy link
Author

slorber commented May 7, 2021

Thanks, let me know how it works.

About the large sidebar, I've opened an issue to track potential perf improvements: facebook/docusaurus#4753

I will also try to figure out how to improve the developer experience, because running docusaurus start on your site is quite painful atm right?

@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

1 similar comment
@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

3 similar comments
@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@github-actions
Copy link

github-actions bot commented May 7, 2021

Preview Site Available

Congratulations! The automatic build has completed succesfully.
A preview site is available at: https://pull-request-616--demisto-content-docs.netlify.app


Important: Make sure to inspect your changes at the preview site.

@glicht
Copy link
Contributor

glicht commented May 7, 2021

Caching seems to be working very good. Reduces the build time from 52 minutes to 12. @sserrata FYI

@slorber
Copy link
Author

slorber commented May 8, 2021

Great news 🤗

@stale
Copy link

stale bot commented May 22, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.

If you'd like to keep it open, please leave a comment with the status of the PR.

Thank you for your contribution!

@stale stale bot added the stale label May 22, 2021
@stale
Copy link

stale bot commented May 29, 2021

This pull request has been automatically closed because there has been no activity for 21 days.

Please feel free to reopen it (or open a new one) if the proposed change is still appropriate.

Thank you for your contribution!

@stale stale bot closed this May 29, 2021
@slorber
Copy link
Author

slorber commented Jun 1, 2021

@glicht FYI we also added a way to use esbuild in the beta.0 , maybe you can try it and see how it improves perf?

It's not yet documented, but we are using it in production on the Docusaurus site itself.

https://github.com/facebook/docusaurus/blob/master/website/docusaurus.config.js#L66

facebook/docusaurus#4765 (comment)

@glicht
Copy link
Contributor

glicht commented Jun 1, 2021

Thanks @slorber. We are planning to upgrade soon. Will check this out.

@glicht glicht mentioned this pull request Jun 17, 2021
@slorber
Copy link
Author

slorber commented Jul 9, 2021

FYI, @glicht discovered we could shave ~12 minutes off the server build time by wrapping the DocSidebar component in BrowserOnly. See https://github.com/demisto/content-docs/blob/master/src/theme/DocSidebar/index.js#L7 for details. It's a small tweak with considerable benefits for large sites.

@sserrata @glicht this Docusaurus PR might make your "sidebar only on the client" less necessary:
facebook/docusaurus#5136

It won't save as much time as simply not rendering the whole sidebar on the server, but seems to me a good compromise. It's up to you to keep your existing optim if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants