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.

Sign up for GitHub

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

Jump to bottom

Use corepack pnpm instead of volta npm #109

Merged
merged 8 commits into from
Nov 1, 2023
Merged

Use corepack pnpm instead of volta npm #109

merged 8 commits into from
Nov 1, 2023

Conversation

jasikpark
Copy link
Collaborator

This updates our node/package manager situation to depend on pnpm and corepack, similar to webclient/www

@jasikpark jasikpark requested a review from IanVS October 31, 2023 15:36
@netlify Netlify
Copy link

netlify bot commented Oct 31, 2023 •
edited
Loading

✅ Deploy Preview for nebula-docs-dn ready!

Name Link
🔨 Latest commit 0c9e576
🔍 Latest deploy log https://app.netlify.com/sites/nebula-docs-dn/deploys/6542647b7d735a0008d335b6
😎 Deploy Preview https://deploy-preview-109--nebula-docs-dn.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.

@jasikpark jasikpark requested a review from johnmaguire October 31, 2023 15:36
@IanVS
Copy link
Collaborator

IanVS commented Oct 31, 2023

Looks like we need to add @types/node to our package.json?

IanVS
IanVS reviewed Oct 31, 2023
View reviewed changes
package.json Outdated
"volta": {
"node": "18.16.0",
"npm": "8.17.0"
"node": ">=20.9.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave this a bit more loose. No reason not to allow node 18+, is there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I think this got lost in a rebase.

@jasikpark jasikpark requested a review from IanVS October 31, 2023 16:58
@jasikpark
Copy link
Collaborator Author 


  

> nebula-docs@0.0.1 typecheck /home/runner/work/nebula-docs/nebula-docs
> tsc

error TS2688: Cannot find type definition file for '@docusaurus/theme-classic'.
  The file is in the program because:
    Entry point of type library '@docusaurus/theme-classic' specified in compilerOptions
 ELIFECYCLE  Command failed with exit code 2.

I can't replicate this locally 🫠

@IanVS
Copy link
Collaborator

IanVS commented Oct 31, 2023

I can. Maybe try clearing your typescript cache?

@IanVS
Copy link
Collaborator

IanVS commented Oct 31, 2023

This looks like a much bigger PR than just moving from volta to pnpm. Let's tone it down a bit, and maybe just use regular npm if pnpm is giving us problems. The main goal is to unblock John.

@jasikpark
Copy link
Collaborator Author

This looks like a much bigger PR than just moving from volta to pnpm. Let's tone it down a bit, and maybe just use regular npm if pnpm is giving us problems. The main goal is to unblock John.

only issue is that corepack doesn't support npm, so looks like John's commit on his own PR is the right way to go then

@IanVS
Copy link
Collaborator

IanVS commented Oct 31, 2023

Alright, then let's get his PR in, then scope this down to just migrating to pnpm?

@jasikpark
Copy link
Collaborator Author

reset to "just pnpm"

IanVS
IanVS approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

I pushed a commit to fix the tsc errors.

jasikpark
jasikpark commented Oct 31, 2023
View reviewed changes
package.json
@@ -18,15 +18,18 @@
"@docusaurus/preset-classic": "2.3.0",
"@mdx-js/react": "^1.6.22",
"clsx": "^1.2.1",
"parse-domain": "^7.0.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why downgrade parse-domain?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The last version that wasn't esm-only.

jasikpark and others added 5 commits October 31, 2023 16:07
@jasikpark
Use corepack pnpm instead of volta npm
3ec718d 
This updates our node/package manager situation to depend on pnpm and corepack, similar to webclient/www
@jasikpark
Fix node versioning file
2dd0ccd
@jasikpark
pnpm format
071b8cc
@jasikpark
use .prettierignore
b070bad
@IanVS @jasikpark
Fix type errors
921f912
@jasikpark jasikpark requested a review from IanVS October 31, 2023 21:08
@jasikpark
Copy link
Collaborator Author

rebased

IanVS
IanVS reviewed Nov 1, 2023
View reviewed changes
.prettierignore

# macOS-specific files
.DS_Store
tsconfig.tsbuildinfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we need to add a prettierignore when moving from npm to pnpm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

b/c it started formatting pnpm-lock.json

README.md Show resolved Hide resolved
package.json Outdated
"volta": {
"node": "18.16.0",
"npm": "8.17.0"
"node": ">=20.9.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I think this got lost in a rebase.

@jasikpark jasikpark requested a review from IanVS November 1, 2023 14:08
IanVS
IanVS approved these changes Nov 1, 2023
View reviewed changes
Copy link
Collaborator

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Readme suggestion, but otherwise looks good.

README.md Outdated

To install our javascript dependencies, run `pnpm install` (or `pnpm i` for short). You'll want to do this whenever you
change branches, if there's a possibility that dependencies have been changed or updated. If you're not able to start up
the client app, this is a good first troubleshooting step to take. To get `pnpm`, run `corepack enable` after installing `node`, and `corepack` should install the right `pnpm` version for you.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend moving this up higher, for those who are following along step-by-step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@jasikpark jasikpark requested a review from IanVS November 1, 2023 14:45
@jasikpark jasikpark merged commit d8a5db0 into main Nov 1, 2023
6 checks passed
@jasikpark jasikpark deleted the update-node-setup branch November 1, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnmaguire johnmaguire Awaiting requested review from johnmaguire

@IanVS IanVS Awaiting requested review from IanVS

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasikpark @IanVS