CLOUDP-352308 Enhanced CLI options for robust linking #3311
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: b5e3774 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
nima-taheri-mongodb
changed the title
nima-taheri-mongodb
changed the title
Contributor
|
Size Change: +2.91 kB (+0.16%) Total Size: 1.79 MB
ℹ️ View Unchanged
|
nima-taheri-mongodb
requested review from
adamrasheed
and removed request for
a team
| .option( | ||
| '--no-parallel', | ||
| 'Run the link command sequentially for each package. Useful for debugging or when the parallel approach fails', | ||
| true, |
Collaborator
There was a problem hiding this comment.
should the default be false (default parallel)?
Collaborator
Author
There was a problem hiding this comment.
It's really confusing, I know. But it works. I could only verify through manual testing that the true value here means the default value for parallel is true.
Collaborator
There was a problem hiding this comment.
Should we change the name of the arg then to --parallel?
Comment on lines
+92
to
+97
| '--launch-env <launchEnv>', | ||
| 'A string of environment variable lines as `KEY=VALUE`, separated by a newline. ' + | ||
| 'Only the specified environment variables will be used during npm link commands in the source and destination directories. ' + | ||
| 'This is useful to workaround environment variable pollution by tools such as version managers (e.g., asdf) or script runners (e.g., pnpm) that interfere with `npm link`. ' + | ||
| 'We recommend using --launch-env="$(env)" to use your original shell environment.', | ||
| undefined, |
Collaborator
There was a problem hiding this comment.
How is this different from just providing KEY=VALUE pnpm run link ... ?
Collaborator
Author
There was a problem hiding this comment.
If we set an environment variable through shell, it will still be overridden by the tool version manager or node package manager.
Both tool version managers (like asdf, nvm, etc.) and Node package managers (such as pnpm, yarn, npm) override environment variables in their own ways to pin their configuration to any process that they spawn. This can cause the link command to fail in the destination folder if it has a different node version configured for it through .tool-versions file or it is configured to use a different node package registry through .npmrc file.
For example, asdf modifies the PATH environment variable by prepending the current nodejs path to it. This forces the link command, when executed in the destination folder, to use the same Node version as the caller, rather than allowing asdf to select the appropriate version based on the destination's .tool-versions file.
Similarly, package managers like pnpm set environment variables such as npm_config_registry, which pins the node package registry to the caller's registry. So the registry specified in the destination folder's .npmrc file will be ignored.
I shared a video walking through this behavior earlier in the LG team's Slack channel if you'd like to see a demo.
https://mongodb.slack.com/archives/CFFA5BS79/p1762399073351499?thread_ts=1762188146.941949&cid=CFFA5BS79
Collaborator
Author
There was a problem hiding this comment.
The solution I proposed in this PR is a trick to pass environment variables to the process in a different way such that doesn't get overridden by the tool version manager or node package manager.
Note that if environment variables are pass in this way, the link command will only use those in the processes it spawns and ignores the current process's own environment variables.
Collaborator
Author
There was a problem hiding this comment.
I'm open to any suggestion, while explaining I actually thought about changing this to --to-launch-env and only apply this to the link command that runs in destination folder 🤔
Collaborator
There was a problem hiding this comment.
Got it, that makes sense. Could have --dest-env and --src-env, though that may add more complexity than it's worth
nima-taheri-mongodb
force-pushed
the
cloudp-352308-robust-link-flags
branch
from
b3416b0 to
b5e3774
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the CLI linking utility with new options to handle edge cases in different development environments. It introduces two flags: --no-parallel to disable parallel linking (defaulting to parallel execution), and --launch-env to provide custom environment variables for spawned processes.
Key Changes:
- Added sequential linking support as an alternative to parallel execution
- Added custom environment variable support for spawned link processes
- Improved package name matching to support both scoped and unscoped formats
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/link/src/utils/linkPackagesForScope.ts | Added parallel/sequential execution logic, environment variable threading, and improved package name matching |
| tools/link/src/utils/linkPackageTo.ts | Added environment variable parameter to link destination function |
| tools/link/src/utils/install.ts | Added environment variable parameter to install function |
| tools/link/src/utils/createLinkFrom.ts | Added environment variable parameter to link source function |
| tools/link/src/link.ts | Added CLI option parsing for parallel flag and launch environment string |
| tools/cli/src/index.ts | Added CLI flag definitions for --no-parallel and --launch-env options |
| DEVELOPER.md | Updated documentation with troubleshooting guidance for linking issues |
| .changeset/giant-phones-jog.md | Added changeset entry documenting the new features |
| #### 6. Publishing additional versions | ||
|
|
||
| To publish additional versions, manually the version number in `packages/<package-name>/package.json`, and re-run step 4. Then, either manually update the external project's `package.json`, or re-run `yarn install @leafygreen-ui/<package-name>`. | ||
| To publish additional versions, manually the version number in `packages/<package-name>/package.json`, and re-run step 4. Then, either manually update the external project's `package.json`, or re-run `pnpm install @leafygreen-ui/<package-name>`. |
There was a problem hiding this comment.
Missing verb in sentence. Should be 'manually update the version number' or 'manually change the version number'.
Suggested change
| To publish additional versions, manually the version number in `packages/<package-name>/package.json`, and re-run step 4. Then, either manually update the external project's `package.json`, or re-run `pnpm install @leafygreen-ui/<package-name>`. | |
| To publish additional versions, manually update the version number in `packages/<package-name>/package.json`, and re-run step 4. Then, either manually update the external project's `package.json`, or re-run `pnpm install @leafygreen-ui/<package-name>`. |
Collaborator
Author
There was a problem hiding this comment.
done. I also noticed that there's another documentation of pnpm link script in README.md along with the documentation of DEVELOPER.md. consolidated them.
Collaborator
Author
There was a problem hiding this comment.
Hey @TheSonOfThomp, I thought about this a bit more and I have an alternative proposal. We use a simple shell script for this job. My reasoning:
- Since this job is essentially a script job, shell is the simplest tool for the job. easy to read, debug, and tweak if needed.
- It doesn't have the environment variable pollution issue that pnpm scripts have.
- We can use
turbo lsscript to fetch the list of packages to link, and use the same--filterof turbo to limit the linking to specific packages (no need for--scopeor--packages)
Here's a POC with almost all features that the current tool has in 140 lines of code:
#!/usr/bin/env bash
set -e
set -o pipefail
target=""
turbo_args=()
npm="pnpm"
parallel=true
cwd=$(pwd)
color_debug="\033[2m" # dim text
color_error="\033[31m" # red text
color_reset="\033[0m" # reset text color
# print the usage message
function print_usage() {
echo "Usage: $0 [--npm | --yarn | --pnpm] [--no-parallel] [--filter=<filter>] <target-path>"
echo ""
echo "Options:"
echo " --npm: Use npm as the package manager"
echo " --yarn: Use yarn as the package manager"
echo " --pnpm: Use pnpm as the package manager"
echo " --no-parallel: Run the link command sequentially for each package. Useful for debugging or when the parallel approach fails"
echo " --filter: Filter the packages to link. Use turbo filter syntax. e.g. '--filter=@lg-charts/*'"
echo " --help: Print this help message"
echo ""
echo "Example: $0 --pnpm --no-parallel --filter='@lg-charts/*' ."
echo "Example: $0 --pnpm --no-parallel --filter='@lg-charts/core' --filter='@lg-tools/*' ."
echo ""
}
# parse the arguments
for arg in "$@"; do
if [[ "$arg" == "--npm" ]]; then
npm="npm"
elif [[ "$arg" == "--yarn" ]]; then
npm="yarn"
elif [[ "$arg" == "--pnpm" ]]; then
npm="pnpm"
elif [[ "$arg" =~ ^--filter= ]]; then
turbo_args+=("$arg")
elif [[ "$arg" == "--no-parallel" ]]; then
parallel=false
elif [[ "$arg" == "--help" ]]; then
print_usage
exit 0
elif [[ "$arg" =~ ^-- ]]; then
echo "Unknown option: $arg"
print_usage
exit 1
else
if [[ -z "$target" ]]; then
target="$arg"
else
echo "Only a single positional argument is supported to specify the target path"
print_usage
exit 1
fi
fi
done
if [[ -z "$target" ]]; then
echo "No target path specified"
print_usage
exit 1
fi
# run a command and print the output with a prefix
function run!() {
# named arguments
local prefix command
local "${@}"
echo -e "${prefix} ${color_debug}→${color_reset} ${command}"
set +e
{
(eval "${command}") \
2> >(while IFS= read -r line; do echo -e "${prefix} ${color_error}$line${color_reset}"; done) \
1> >(while IFS= read -r line; do echo -e "${prefix} ${color_debug}$line${color_reset}"; done)
}
local exit_code=$?
set -e
if [[ $exit_code -ne 0 ]]; then
echo -e "${prefix} ${color_debug}✖${color_reset} ${color_error}failed with exit code $exit_code${color_reset}"
else
echo -e "${prefix} ${color_debug}✔${color_reset} ${color_debug}finished successfully${color_reset}"
fi
return $exit_code
}
function get_random_color() {
local color_options=(
"\033[32m" # Green
"\033[33m" # Yellow
"\033[34m" # Blue
"\033[35m" # Magenta
"\033[36m" # Cyan
"\033[92m" # Bright Green
"\033[93m" # Bright Yellow
"\033[94m" # Bright Blue
"\033[95m" # Bright Magenta
"\033[96m" # Bright Cyan
)
echo "${color_options[$((RANDOM % ${#color_options[@]}))]}"
}
# link a package from the from_path to the to_path
function link_package() {
# named arguments
local name from_path to_path
local "${@}"
local prefix="$(get_random_color)[$name]${color_reset}"
run! prefix="$prefix" command="cd $from_path && $npm link"
run! prefix="$prefix" command="cd $to_path && $npm link $name"
}
packages=$($npm turbo ls ${turbo_args[@]} --output=json | jq -r '.packages.items[]? | "\(.path)|\(.name)"')
running_pids=()
for package in $packages; do
IFS='|' read -r path name <<<"$package"
if [[ $parallel == true ]]; then
link_package name="$name" from_path="$path" to_path="$target" &
running_pids+=($!)
else
link_package name="$name" from_path="$path" to_path="$target"
fi
done
if [[ $parallel == true ]]; then
for pid in "${running_pids[@]}"; do
wait "$pid"
done
fi
Collaborator
There was a problem hiding this comment.
I like the idea, but I think I'd prefer keeping this a JS script, for the simple reason that the maintainers of this repo are primarily JS devs, and maintaining that would be easier than a complex shell script
Collaborator
Author
There was a problem hiding this comment.
Fair point! Quick story—at my last company (Hootsuite), our team initially wrote a lot of our build and dev-tooling scripts in SBT/Scala since that was our main language. But as time went on, we found Scala's power actually led to over-complicated scripts that were tough to debug and maintain. Eventually, we decided to migrate everything back to long but simple shell scripts and found things easier to manage.
I did not plan this at all 😂 but #3319 just emerged to help my side of argument.
Another point in favor of shell scripts is that modern DevOps culture expects developers to be comfortable with operational tasks including scripting, so having basic shell scripting skills is pretty much essential. Plus, with LLMs available, writing and debugging shell scripts is easier than ever.
So I'm still a fan of shell scripts for these kinds of tools, but I'll defer to whatever your team prefers!
Contributor
|
Coverage after merging cloudp-352308-robust-link-flags into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Chain of upstream PRs as of 2025-11-12
PR CLOUDP-352308 add logging for spawn processes in link tools #3310:
main←cloudp-352308-spawn-loggedcloudp-352308-spawn-logged←cloudp-352308-robust-link-flags🎫 Ticket
CLOUDP-352308
📝 Description
This PR introduces new CLI flags to improve the flexibility of the linking process.
✨ New Features
--no-parallelflag: Run link commands sequentially instead of parallel execution. Useful for debugging or when parallel linking fails due to race conditions.--launch-envflag: Specify custom environment variables for spawned processes. This helps work around environment pollution from version managers (likeasdfornvm) or script runners (likepnpm). Recommended usage:--launch-env="$(env)"to use your original shell environment.🧪 Documentation and Testing
DEVELOPER.mdwith troubleshooting guidancepnpm changesetand documented my changes--no-parallelflag--launch-envflag