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

Replace npm with pnpm #2545

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Replace npm with pnpm #2545

merged 2 commits into from
Dec 7, 2021

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Nov 29, 2021

Issue This PR Addresses

Initial work for #1778

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR changes the package manager from npm to pnpm. pnpm is faster and uses less disk space.

To try this locally:

  • clone the telescope repo to your computer (do not use your current local copy of telescope)
  • install pnpm: npm install -g pnpm
  • run: pnpm install

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 29, 2021

@menghif
Copy link
Contributor Author

menghif commented Nov 29, 2021

I'm working on updated GitHub Actions for this.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I've pulled this locally and tested, everything worked for me.

We should use pnpm and set its cache in our workflows, see https://pnpm.io/continuous-integration#github-actions.

We need to update all of our docs, which will refer to npm. Managing the transition to pnpm from npm is going to take education and docs.

.prettierignore Outdated Show resolved Hide resolved
@menghif menghif marked this pull request as ready for review November 29, 2021 19:53
.husky/pre-commit Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/test-web-content/manual-auth/index.html Outdated Show resolved Hide resolved
@menghif
Copy link
Contributor Author

menghif commented Nov 29, 2021

When I run eslint, I get this message:

> @senecacdot/telescope@2.3.0 eslint /Users/francesco/telescope
> eslint --config .eslintrc.js "**/*.{jsx,tsx,ts,js}"

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <4.3.0

YOUR TYPESCRIPT VERSION: 4.5.2

Please only submit bug reports when using the officially supported version.

=============

followed by:

✖ 974 problems (974 errors, 0 warnings)

@humphd
Copy link
Contributor

humphd commented Nov 29, 2021

@DukeManh was just updating eslint, do you need to rebase?

@menghif
Copy link
Contributor Author

menghif commented Nov 29, 2021

@DukeManh was just updating eslint, do you need to rebase?

I already rebased this morning to get @DukeManh's changes. This is a problem related to pnpm

@humphd
Copy link
Contributor

humphd commented Nov 29, 2021

@humphd
Copy link
Contributor

humphd commented Nov 29, 2021

I wonder if this is some latent dev dependency bug, where pnpm is not doing the same thing as npm with respect to pulling in peer deps.

@menghif

This comment has been minimized.

@humphd
Copy link
Contributor

humphd commented Nov 30, 2021

What errors do you have? What didn't it import?

One thing we might be hitting here is how we structured our dependencies using npm. We started out with a single project, what is now src/backend. When that was the case, we put everything in package.json in the root. As we grew other sub-projects, we started adding other package.json files, but tried to put common dependencies in the root package.json file.

With pnpm this isn't necessary any more. I think we should creating a package.json for src/backend and have the root package.json only contain things like jest, eslint, and other dev stuff. We could also move the root Dockerifile to src/backend. Doing so might fix some of this. cc @manekenpix

@menghif
Copy link
Contributor Author

menghif commented Nov 30, 2021

Sorry looks like the problem was due to the fact that I removed npm-run-all from the dependencies.

I thought that npm-run-all was only used here:

"postinstall": "run-s install:*",

and here:
"test-ci": "run-s prettier-check test",

Is it being used anywhere else?


With pnpm this isn't necessary any more. I think we should creating a package.json for src/backend and have the root package.json only contain things like jest, eslint, and other dev stuff. We could also move the root Dockerifile to src/backend. Doing so might fix some of this.

Yes this makes sense, I will work on it.

@humphd
Copy link
Contributor

humphd commented Nov 30, 2021

We call it via run-s, see https://github.com/Seneca-CDOT/telescope/search?q=run-s

package.json Outdated Show resolved Hide resolved
test/test-web-content/manual-auth/index.html Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@menghif
Copy link
Contributor Author

menghif commented Nov 30, 2021

@humphd I didn't know how to set up the cache in GitHub Actions so I took from your old PR. I am not able to push any changes related to workflows from my computer so I am doing it via the GitHub website.

package.json Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Dec 1, 2021

The cache is working (see Actions run here)

@humphd
Copy link
Contributor

humphd commented Dec 1, 2021

This took ~11m in CI vs. 15-30m it usually takes, so already a win.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

We really need the docs updated, so we don't cause chaos when we merge this.

.github/workflows/node-js-ci.yml Show resolved Hide resolved
docs/environment-setup.md Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Dec 3, 2021

@manekenpix will landing this break deployment, since it requires a global install of pnpm? If so, we should co-ordinate a time to do that on staging/prod.

@humphd
Copy link
Contributor

humphd commented Dec 3, 2021

I suspect we need to fix #2563 before we can land this.

@manekenpix
Copy link
Member

@humphd these changes don't affect staging and production deployment, so I think they're safe to merge once the PR passes all the tests.

@humphd
Copy link
Contributor

humphd commented Dec 5, 2021

What's our plan to move this forward?

@menghif
Copy link
Contributor Author

menghif commented Dec 5, 2021

I'm hoping that #2563 will fix this. I'll work on it.

@menghif
Copy link
Contributor Author

menghif commented Dec 7, 2021

I rebased, updated pnpm-lock.yaml and now CI works!

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

🥳 Really glad you stuck with this.

@humphd humphd merged commit e9d694a into Seneca-CDOT:master Dec 7, 2021
@humphd
Copy link
Contributor

humphd commented Dec 7, 2021

@menghif can you get that pnpm post up and link it here so I can send to all students today?

@menghif
Copy link
Contributor Author

menghif commented Dec 7, 2021

@menghif can you get that pnpm post up and link it here so I can send to all students today?

https://dev.to/menghif/transition-to-pnpm-4alf

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.

6 participants