-
Notifications
You must be signed in to change notification settings - Fork 107
feat(many): migrate from npm to pnpm Phase 1 #2172
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
Conversation
|
7c5ddb5 to
339bb0a
Compare
ffdc140 to
b850083
Compare
b850083 to
57b6c70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Implements Phase 1 of pnpm migration with workspace protocol and core tooling updates. Migrate from npm to pnpm v10.18.3 as the package manager. All packages now use the workspace:* protocol for internal dependencies, which pnpm automatically resolves to actual versions on publish.
- Package manager configuration with pnpm-workspace.yaml and .npmrc
- Convert all 94 internal @instructure/* dependencies from exact versions to workspace:*
- Update build & tooling scripts to use pnpm commands and fix workspace module resolution
Reviewed Changes
Copilot reviewed 171 out of 177 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Define workspace packages configuration |
| lerna.json | Update to use pnpm as npm client |
| package.json | Add pnpm engines, overrides, devDependencies and update scripts |
| scripts/clean.js | Optimize performance with parallel deletions and native shell commands |
| scripts/bootstrap.js | Update to use pnpm and sequential builds |
| packages/*/package.json | Convert internal dependencies to workspace:* protocol |
| regression-test/* | Decouple workspace using link: protocol and update imports |
| babel-plugin-transform-imports | Fix for pnpm workspace module resolution |
| docs/* | Update documentation to use pnpm commands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| export default function ButtonPage() { | ||
| const myElementRef = useRef<HTMLButtonElement>(null) | ||
| const myElementRef = useRef<any>(null) |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ref type should be more specific than any. Use HTMLButtonElement | null for better type safety.
| const myElementRef = useRef<any>(null) | |
| const myElementRef = useRef<HTMLButtonElement | null>(null) |
| ] as const | ||
| const sizes = ['small', 'medium', 'large'] as const |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider defining these arrays outside the component to avoid recreation on each render, improving performance.
| ))} | ||
| </div> | ||
| <Button renderIcon={IconAddLine}>Icon Button</Button> | ||
| <Button renderIcon={<IconAddLine />}>Icon Button</Button> |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icon should be passed as a component reference rather than an element to allow the Button component to control rendering. Use renderIcon={IconAddLine} instead.
| <Button renderIcon={<IconAddLine />}>Icon Button</Button> | |
| <Button renderIcon={IconAddLine}>Icon Button</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm run devruns, but the docs app does not load for me (its stuck in loading, like it would be blocked by a firewall- there are a couple of
npmreferences left, e.g."build:watch": "npm run ts:checkis in lots of places - Please double check
ui-scripsscripts, e.g.publish.js,tag.jsstill usesnpm,deprecate,dangerfile.js
| if (matches && matches[1]) { | ||
| const packageName = matches[1] | ||
| const sourceIndex = require.resolve(packageName) | ||
| // For pnpm workspaces, resolve from the project root to find workspace packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed with pnpm?
| if (args.length > 0 && args[0] === '--nuke_node') { | ||
| // eslint-disable-next-line no-console | ||
| console.info('Deleting node_modules recursively...') | ||
| removeNodeModules() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? Can there be node_modules folders in packages?
this turned out to be a harder issue than anticipated, i think something happened with webpack processing the symlinked files that pnpm uses which causes an infinite loop, i'm investigating... |
|
There are still some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments. I've tested most scripts, all seem to work, nice job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job, I tried most of the commands, they work well
please check the ui-scripts folder for npm and change those too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 179 out of 185 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| execSync( | ||
| 'find . -name "node_modules" -type d -prune -exec rm -rf {} + 2>/dev/null || true', | ||
| { stdio: 'inherit' } | ||
| ) |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell command uses unescaped user input from process.cwd() context and relies on shell globbing. On Windows or with special characters in paths, this could fail or behave unexpectedly. Consider using Node.js APIs instead of shell commands for cross-platform reliability.
| "prepare-build": "npm run build-icons", | ||
| "build-icons": "ui-scripts build-icons --svgoConfig svgo.config.js --config icons.config.js", | ||
| "prepare-build": "pnpm run build-icons", | ||
| "build-icons": "pnpm exec ui-scripts build-icons --skipOptimization --config icons.config.js", |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --skipOptimization flag is used by default with a TODO comment indicating this is temporary. Consider creating a separate script for optimized builds (like build-icons:optimize) and making the default behavior explicit rather than temporary.
| // If any transform fails, we can't safely transform this import | ||
| allTransformsSucceeded = false | ||
| } |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a transform fails, allTransformsSucceeded is set to false but the loop continues processing remaining imports. This could lead to partial transforms in the memberTransforms array that are later discarded. Consider breaking early or restructuring to avoid unnecessary work.
package.json
Outdated
| "react": "^18", | ||
| "react-dom": "^18", | ||
| "@types/react": "^18" |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pnpm overrides use caret ranges (^18) which allow minor and patch updates, while devDependencies specify exact versions (18.3.1). This inconsistency could lead to version mismatches. Consider using exact versions in overrides to match the devDependencies, or document why flexible versions are needed.
| "react": "^18", | |
| "react-dom": "^18", | |
| "@types/react": "^18" | |
| "react": "18.3.1", | |
| "react-dom": "18.3.1", | |
| "@types/react": "18.3.26" |
| const packageMatch = sourceTokens.match( | ||
| /^(@instructure\/)?([\w-]+)(.*)$/ | ||
| ) |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The regex pattern [\w-]+ matches word characters and hyphens, but doesn't account for scoped package names with slashes or special characters that might exist in subpaths. Consider a more explicit pattern or validate the expected format in the error message.
regression-test/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "instructure-ui": "file:../packages", | ||
| "@instructure/ui": "link:../packages/ui", |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The regression-test uses link: protocol instead of workspace:* like other packages. While this may be intentional for the separate workspace, consider documenting why a different protocol is used here compared to the main workspace pattern.
| function debouncedProcess(filePath) { | ||
| const now = Date.now() | ||
| const lastProcessed = processedFiles.get(filePath) | ||
|
|
||
| if (lastProcessed && now - lastProcessed < DEBOUNCE_MS) { | ||
| return // Skip if file was processed recently | ||
| } |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debounce implementation doesn't clear old entries from the processedFiles Map, which will grow unbounded during long-running watch sessions. Consider periodically cleaning up entries older than DEBOUNCE_MS or using a proper debounce library.
scripts/clean.js
Outdated
|
|
||
| const fs = require('fs') | ||
| const fs = require('fs').promises | ||
| const fsSync = require('fs') |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable fsSync.
dd7cc43 to
6d8ae68
Compare
8536b5f to
0c60fe4
Compare
Implement pnpm migration with workspace protocol and core tooling updates: Configuration: - Add pnpm-workspace.yaml defining workspace packages - Add .npmrc with pnpm configuration (hoisted node linker, strict peer deps) - Update lerna.json to use pnpm as npm client - Update root package.json with pnpm engines, overrides, and devDependencies - Update .gitignore for pnpm artifacts - Delete package-lock.json (replaced by pnpm-lock.yaml) Workspace Dependencies: - Convert all internal @instructure/* dependencies from exact versions to workspace:* - Update 94 package.json files across all packages - Create scripts/convert-to-workspace-protocol.js for automated conversion Build Tooling: - Update scripts/bootstrap.js to use pnpm and run builds sequentially - Update packages/ui-scripts/lib/commands/bump.js to use pnpm install - Update packages/ui-scripts/lib/utils/npm.js to use pnpm whoami - Fix babel-plugin-transform-imports for pnpm workspace module resolution - Fix generate-all-tokens for pnpm workspace package resolution - Optimize clean script performance - Restore markdown hot reload in dev server Regression Test App (npm): - Keep regression-test using npm to simulate external package consumption - Change @instructure/ui dependency to file: protocol (from link:) - Remove pnpm-lock.yaml and pnpm-workspace.yaml - Generate package-lock.json with npm install - Update .github/workflows/visual-regression.yml to use npm commands - Update README.md with npm commands and explanation - Add .npmrc explaining npm-only approach Documentation: - Update all internal npm references to pnpm across the codebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
0c60fe4 to
474e8bd
Compare
Summary
Implement Phase 1 of pnpm migration with workspace protocol and core tooling updates. Migrate from npm to pnpm v10.18.3 as the package manager. All packages now use the
workspace:*protocol for internal dependencies, which pnpm automatically resolves to actual versions on publish.Changes
Package Manager Configuration
Workspace Dependencies
Build & Tooling
@types/reactandjsdomto root devDependencies (were phantom dependencies)--nuke_nodemode with native shell command (10-100x faster)onListeningcallback from firingregression-test Workspace Decoupling
link:protocol instead of workspace overlap@instructure/uibundleCI/CD
Documentation
Breaking Changes
None for package consumers.
For contributors: install pnpm and use
pnpmcommands instead ofnpm.Test plan
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]