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

fix: cli upgrade helper fail when no package.dependencies #5204

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

mweststrate
Copy link
Contributor

Motivation

In Flipper, https://github.com/facebook/flipper/blob/master/website/package.json, all our dependencies live in devDependecies of our package.json. As a result dependencies is not set, causing a crash when running yarn start:

/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/@docusaurus/core/bin/docusaurus.js:50
  const siteDocusaurusPackagesForUpdate = Object.keys(sitePkg.dependencies)
                                                 ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.<anonymous> (/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/@docusaurus/core/bin/docusaurus.js:50:50)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:991:10)
    at Module.load (internal/modules/cjs/loader.js:811:32)
    at Function.Module._load (internal/modules/cjs/loader.js:723:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1043:10)
    at internal/main/run_main_module.js:17:11
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Setting "dependencies": {}, works as work around, but this patch fixes the crash, and makes sure the deps are upgraded as well if they live in devDependencies instead of dependencies.

Have you read the Contributing Guidelines on pull requests?

skimmed

Test Plan

yarn install && yarn start in the flipper website/ dir, per link above.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

In Flipper, https://github.com/facebook/flipper/blob/master/website/package.json,  all our dependencies live in `devDependecies` of our `package.json`. As a result `dependencies` is not set, causing a crash when running `yarn start`:

```
/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/@docusaurus/core/bin/docusaurus.js:50
  const siteDocusaurusPackagesForUpdate = Object.keys(sitePkg.dependencies)
                                                 ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.<anonymous> (/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/@docusaurus/core/bin/docusaurus.js:50:50)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:991:10)
    at Module.load (internal/modules/cjs/loader.js:811:32)
    at Function.Module._load (internal/modules/cjs/loader.js:723:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1043:10)
    at internal/main/run_main_module.js:17:11
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
```

Setting `"dependencies": {}`, works as work around, but this patch fixes the crash, and makes sure the deps are upgraded as well if they live in `devDependencies` instead of `dependencies`.
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 22, 2021
@netlify
Copy link

netlify bot commented Jul 22, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: d35f470

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60f944ca2ade84000754bdfa

😎 Browse the preview: https://deploy-preview-5204--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jul 22, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 95
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5204--docusaurus-2.netlify.app/

facebook-github-bot pushed a commit to facebook/flipper that referenced this pull request Jul 22, 2021
Summary:
`yarn start` in `website` crashed with

```
/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/docusaurus/core/bin/docusaurus.js:50
  const siteDocusaurusPackagesForUpdate = Object.keys(sitePkg.dependencies)
                                                 ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Object.<anonymous> (/Users/mweststrate/fbsource/xplat/sonar/website/node_modules/docusaurus/core/bin/docusaurus.js:50:50)
```

This diff adds an empty `dependencies` section as work around. Proper patch has been made in:

facebook/docusaurus#5204

Reviewed By: nikoant

Differential Revision: D29844597

fbshipit-source-id: 29cfad53d9ca785dd9d93c4800647add219a48ba
@slorber
Copy link
Collaborator

slorber commented Jul 22, 2021

LGTM thanks 👍

Was wondering why you put docusaurus packages as devDependencies? It's unusual

@slorber slorber changed the title Fix handling of empty dependencies section. fix: cli upgrade helper fail when no package.dependencies Jul 22, 2021
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jul 22, 2021
@slorber slorber merged commit ddc0f46 into facebook:master Jul 22, 2021
@mweststrate
Copy link
Contributor Author

Just checked, doesn't seem to be a specific rationale behind it, but just there as devDependency since original generation early 2018 with docusaurus 1.0.9. Semantically speaking I'd say it is quite correct to have them as devDependencies, since these are all(?) build-only deps. But that gets into the nit picking territory 😅.

....Unless dependabot would finally stop running false security errors havoc on devDependencies, then it would suddenly add a lot of value to use those instead of normal dependencies 😆

@slorber
Copy link
Collaborator

slorber commented Jul 23, 2021

I see thanks :)

I guess it's a matter of perspective. IMHO for an SSG we can consider the CI (Netlify/Vercel/GH actions) is the production env, and can leverage yarn install --production to install faster and skip pure dev things like eslint and prettier :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants