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(core): replace wait-on dependency with custom lighter code #9547

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

NickGerleman
Copy link
Contributor

@NickGerleman NickGerleman commented Nov 14, 2023

Pre-flight checklist

Motivation

This change removes usage of wait-on which brings in a few dependencies it ideally wouldn't, and replaces it with an effective copy of the algorithm and APIs it takes under the hood for current usage.

  1. wait-on sees a file as present when fs.stat on the path stops throwing
  2. It polls on a timer (which WaitPlugin sets to 300ms)
  3. It waits until a time has passed without file size changing (defaults to 750ms)
  4. wait-on defaults to no timout, so we poll forever.

See https://github.com/jeffbski/wait-on/blob/master/lib/wait-on.js#L200 for reference

Test Plan

  1. Copy the waitOn function to a standalone Node script
  2. call await waitOn('foo/bar.js') with CWD of /Users/ngerlem/github/docusaurus
  3. Create /Users/ngerlem/github/docusaurus/foo
  4. Touch /Users/ngerlem/github/docusaurus/bar.js

Promise is resolved shortly after.

WaitPlugin as invoked by build does not hang.

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

  1. Replace wait-on dependency #9537

Fixes facebook#9537

This change removes usage of `wait-on`, and replaces it with an effective copy of the algorithm it ends up taking for our use case.

1. `wait-on` sees a file as present when `fs.stat` on the path stops throwing
2. It polls on a timer (which WaitPlugin sets to 300ms)
3. It waits until a time has passed without file size changing (defaults to 750ms)
4. `wait-on` defaults to no timout, so we poll forever.

See https://github.com/jeffbski/wait-on/blob/master/lib/wait-on.js for reference
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 14, 2023
@NickGerleman NickGerleman changed the title [DRAFT] chore: Remove wait-on dependency [WIP] chore: Remove wait-on dependency Nov 14, 2023
Copy link

netlify bot commented Nov 14, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 3fb71b3
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/655b682b9cd0f30008b42c5a
😎 Deploy Preview https://deploy-preview-9547--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Nov 14, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 69 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 88 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 74 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 65 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 70 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 74 🟢 100 🟢 100 🟢 90 🟠 89 Report

@NickGerleman NickGerleman changed the title [WIP] chore: Remove wait-on dependency chore: remove wait-on dependency Nov 14, 2023
@NickGerleman NickGerleman marked this pull request as ready for review November 14, 2023 08:51

async function waitOn(filepath: string): Promise<void> {
const pollingIntervalMs = 300;
const stabilityWindowMs = 750;
Copy link
Contributor Author

@NickGerleman NickGerleman Nov 14, 2023

Choose a reason for hiding this comment

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

This is the previous logic, but it seems a little bit fragile, or prone to add delay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as no regression, we'll take it! It should also be much more trivial to make it more robust now :)

@Josh-Cena Josh-Cena added the pr: dependencies Pull requests that update a dependency file label Nov 15, 2023
@slorber
Copy link
Collaborator

slorber commented Nov 20, 2023

LGTM thanks 👍

@slorber slorber changed the title chore: remove wait-on dependency chore(core): replace wait-on dependency with custom lighter code Nov 20, 2023
@slorber slorber merged commit 424ffd2 into facebook:main Nov 20, 2023
30 checks passed
@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Nov 20, 2023
Copy link

argos-ci bot commented Nov 20, 2023

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 🧿 Changes detected (Review) 1 change Nov 30, 2023, 6:57 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace wait-on dependency
4 participants