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

Migrate to pnpm #11358

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Migrate to pnpm #11358

merged 1 commit into from
Mar 21, 2024

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Mar 21, 2024

I've added PR comments highlighting the notable changes below.

Copy link

changeset-bot bot commented Mar 21, 2024

⚠️ No Changeset found

Latest commit: 282d27c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -17,5 +17,6 @@
"access": "public",
"baseBranch": "dev",
"updateInternalDependencies": "patch",
"bumpVersionsWithWorkspaceProtocolOnly": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

This setting is now enabled so that Changesets doesn't attempt to bump the react-router-dom dependency in react-router-dom-v5-compat since it's set to "4 || 5". This is a simpler method of achieving what the postversion.mjs script was doing since we can now use workspace dependencies.

Comment on lines -1 to -23
import * as path from "path";
import { createRequire } from "node:module";

// Changesets assumes that an internal peer dependency's version range should
// include the version of that package in our repo. This normally makes sense,
// but the compat package has a peer dependency on react-router-dom v4 or v5, so
// we need to:
// - Avoid validity checks for peer dependencies (done via patch package)
// - Reset the automatic version updates resulting from yarn changeset version
// (done via a simple node script)

const require = createRequire(import.meta.url);
const jsonfile = require("jsonfile");

let file = path.join(
process.cwd(),
"packages/react-router-dom-v5-compat/package.json"
);
let json = await jsonfile.readFile(file);
json.peerDependencies["react-router-dom"] = "4 || 5";
await jsonfile.writeFile(file, json, { spaces: 2 });

console.log("Patched peerDependencies for react-router-dom-v5-compat");
Copy link
Member Author

Choose a reason for hiding this comment

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

This script can now be removed because we've enabled the Changesets bumpVersionsWithWorkspaceProtocolOnly setting, so this version range isn't bumped by Changesets anymore and doesn't need to be fixed after running changeset version.

"scripts": {
"build": "rollup -c",
"clean": "git clean -fdX .",
"format": "prettier --ignore-path .eslintignore --write .",
"format:check": "prettier --ignore-path .eslintignore --check .",
"postinstall": "patch-package",
Copy link
Member Author

Choose a reason for hiding this comment

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

This feature is built into pnpm: https://pnpm.io/cli/patch

@@ -0,0 +1,2 @@
ignore-workspace-cycles=true
Copy link
Member Author

Choose a reason for hiding this comment

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

This is suppressing dependency warnings that only affect dev dependencies due to test code. Everything still works.

@@ -0,0 +1,2 @@
ignore-workspace-cycles=true
enable-pre-post-scripts=true
Copy link
Member Author

Choose a reason for hiding this comment

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

This is optional, but I've opted to keep automatic pre and post script behaviour so that the repo doesn't change too much in the migration. We can revisit this later.

Comment on lines -4 to -12
"workspaces": {
"packages": [
"packages/react-router",
"packages/react-router-dom",
"packages/react-router-dom-v5-compat",
"packages/react-router-native",
"packages/router"
]
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now configured in pnpm-workspace.yaml.

modulePaths: [
"<rootDir>/node_modules", // for react-native
],
transformIgnorePatterns: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

The modulePaths option as configured didn't work with pnpm's folder structure. A simpler way to solve the same problem is to simply tell Jest to transform everything.

@@ -1,5 +1,6 @@
/fixtures/
node_modules/
pnpm-lock.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed so that pnpm format doesn't format the lock file. We ran into an issue in the Remix repo where CI got stuck in a loop deduping and formatting pnpm-lock.yaml since pnpm and Prettier didn't agree, so this ensures we don't hit that problem again.

@@ -29,7 +29,7 @@ import type {
Location,
RouteObject,
To,
} from "react-router-dom";
} from "./index";
Copy link
Member Author

Choose a reason for hiding this comment

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

pnpm didn't let us get away with our original approach.

@jacob-ebey jacob-ebey merged commit 12afb2e into dev Mar 21, 2024
3 checks passed
@jacob-ebey jacob-ebey deleted the pnpm branch March 21, 2024 16:36
Copy link
Contributor

🤖 Hello there,

We just published version 6.23.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.23.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants