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

feat: migrate from yarn to pnpm #567

Merged
merged 19 commits into from Mar 13, 2024
Merged

feat: migrate from yarn to pnpm #567

merged 19 commits into from Mar 13, 2024

Conversation

milesj
Copy link
Contributor

@milesj milesj commented Mar 12, 2024

Description

Part of #543

This PR does a bunch of things:

  • Migrates from yarn to pnpm. I ran some small benchmarks and pnpm was about 3x faster on cold installs, but the same on warm installs.
yarn 4.1.1 pnpm 8.15.4
Cold 14.42s 4.5s
Warm 1.45s 1.3s
  • Audited dependencies of every package.json, and removed deps that weren't used, or should be in the root.
  • Tested all scripts, and fixed any issues I encountered.

Test Plan


Copy link

netlify bot commented Mar 12, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit c33b66a
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/65f1a5c26f89720008215ee5

package.json Outdated
"scripts": {
"nuke": "rm -rf node_modules && rm -rf packages/*/node_modules && rm -rf web/*/node_modules && rm -rf examples/*/node_modules && rm -rf crates/rolldown_binding_wasm/node_modules && rm -rf scripts/node_modules",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For testing...

@@ -105,6 +106,15 @@ async function runRollup(item) {
for (const suite of suites) {
const bench = new Bench({ time: 100, iterations: suite.benchIteration ?? 10 })

// Check if inputs have been initialized
for (const input of suite.inputs) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing, this would crash hard if just setup-bench wasn't ran, so added this little check.

"@types/node": "^20.11.25",
"npm-run-all2": "^6.1.2",
"prettier": "^3.2.5",
"colorette": "^2.0.20",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are used in tests, but weren't listed. Because pnpm scopes them, they're now required.

@@ -1,4 +1,4 @@
import type { RollupOptions, RollupOutput } from 'rolldown'
import type { RollupOptions } from '../../../../src'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of pnpm, rolldown was no longer able to reference itself, so had to use relative paths.

Comment on lines 103 to 107
- name: Install pnpm
run: corepack enable

- name: Install pnpm
run: corepack enable
Copy link
Contributor

@Demivan Demivan Mar 13, 2024

Choose a reason for hiding this comment

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

This is duplicated.
actions/cache needed to cache pnpm packages, so they are not downloaded every time.

@milesj
Copy link
Contributor Author

milesj commented Mar 13, 2024

Dependencies install in less than 45 seconds, typically around 15-20. Not really worth adding caching at this point.

@Demivan
Copy link
Contributor

Demivan commented Mar 13, 2024

Dependencies install in less than 45 seconds, typically around 15-20. Not really worth adding caching at this point.

Why not make it even faster? It is just 4–5 lines of CI code.

@hyf0
Copy link
Member

hyf0 commented Mar 13, 2024

@milesj Do you have time to clean up this PR? So we could merge the migration first. We could solve release problems in another PR.


I saw the c-spell job failed. You could add the issue file to cspell.json#ignorePaths to ignore it.

@hyf0
Copy link
Member

hyf0 commented Mar 13, 2024

Let me take it from here. I will comment out the release ci for temp.

@hyf0 hyf0 changed the title WIP: Migrate from yarn to pnpm. Rework release workflow. feat: migrate from yarn to pnpm Mar 13, 2024
Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

Great work!

@hyf0 hyf0 merged commit fbe32d2 into main Mar 13, 2024
10 checks passed
@hyf0 hyf0 deleted the release-flow branch March 13, 2024 13:13
@milesj
Copy link
Contributor Author

milesj commented Mar 13, 2024

Downloading and unpacking an archive over the network will still take similar times. Caching isn't really necessary.

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.

None yet

3 participants