Remove dotenv from cli-kit; use native util.parseEnv#7728
Open
amcaplan wants to merge 4 commits into
Open
Conversation
amcaplan
added
dependency-removal
github-actions
Bot
added
no-changelog
amcaplan
force-pushed
the
remove-dep/dotenv
branch
2 times, most recently
from
67c3306 to
50a2bba
Compare
The rule's `knownFailures` allowlist is keyed by repo-root-relative paths, so it must compute the repo root to build each lookup key. It derived the root with a fixed `../../../../../../..` offset from this rule's `__dirname`, which assumes a specific install depth. The package manager doesn't guarantee that: pnpm places the plugin as a deep `file:` copy or a `link:` symlink depending on peer resolution, and a shift in that layout makes the offset compute the wrong root, breaking every key lookup and flagging all grandfathered files. Ask git for the repo root instead (`git rev-parse --show-toplevel`), which is correct regardless of install layout. The rule only runs in dev/CI, where git is always present. Memoized since the root is constant for a lint run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Node's built-in `util.parseEnv` (available since v20.12.0) replaces the `dotenv` dependency for parsing `.env` files in cli-kit. Bump `@types/node` to 22.x to match the `engines.node >=22.12.0` requirement, which lets us drop the local `parseEnv` type override; `parseEnv` types values as `string | undefined`, so the result is narrowed to `Record<string, string>` at the single call site (it never yields undefined at runtime). The `@types/node` bump widens `process.exit`'s signature, so two theme test mocks are updated to match. The e2e package keeps its own `dotenv` dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
amcaplan
force-pushed
the
remove-dep/dotenv
branch
from
50a2bba to
a9b6b42
Compare
amcaplan
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the dotenv dependency from @shopify/cli-kit by switching .env parsing to Node’s native util.parseEnv(), bumps @types/node to align with the repo’s Node engine requirement, and fixes an ESLint rule’s repo-root detection to be resilient to pnpm install layouts.
Changes:
- Replace
dotenv.parse()withnode:util.parseEnv()in@shopify/cli-kitand dropdotenvfrom its dependencies. - Bump
@types/nodefrom18.19.70to22.19.17(root +packages/e2e) and adjust theme tests’process.exitmocks accordingly. - Update
no-inline-graphqlESLint rule repo-root lookup to usegit rev-parse --show-toplevelrather than a fixed__dirnameoffset.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile for @types/node bump, cli-kit dependency removal, and pnpm link layout changes. |
| packages/theme/src/cli/utilities/theme-environment/theme-polling.test.ts | Updates process.exit mock signature to match new Node typings. |
| packages/theme/src/cli/utilities/errors.test.ts | Updates process.exit mock signature to match new Node typings. |
| packages/eslint-plugin-cli/rules/no-inline-graphql.js | Switches repo-root detection to a git-based approach to avoid install-depth assumptions. |
| packages/e2e/package.json | Bumps @types/node to match the repo’s Node engine baseline. |
| packages/cli-kit/src/public/node/dot-env.ts | Replaces dotenv parsing with util.parseEnv() and narrows the resulting type. |
| packages/cli-kit/package.json | Removes dotenv dependency from @shopify/cli-kit. |
| package.json | Bumps root @types/node version. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+34
| // parseEnv types values as `string | undefined`, but it never yields undefined values at runtime. | ||
| variables: parseEnv(content) as Record<string, string>, |
…lookup - dot-env: drop undefined values from parseEnv output so DotEnvFile.variables truly matches Record<string, string> instead of casting the type away - no-inline-graphql: fall back to process.cwd() when git is unavailable or run outside a repo, so linting keeps working instead of crashing Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| "@shopify/toml-patch": "0.3.0", | ||
| "@types/node": "18.19.70", | ||
| "@types/node": "22.19.17", | ||
| "dotenv": "16.4.7", |
Contributor
There was a problem hiding this comment.
We should remove this one as well
Contributor
Author
There was a problem hiding this comment.
Good call, will do!
cli-kit already dropped dotenv in favor of Node's util.parseEnv, but the e2e package was still depending on it directly. Replace dotenv's config() with a loadEnvFile() helper in setup/env.ts built on util.parseEnv, preserving the same semantics: a missing .env file is silently ignored and values already present in process.env are never overwritten. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Remove
dotenvfrom@shopify/cli-kitWhy: Part of an initiative to cut low-value dependency churn (10 Dependabot bumps / 24 months).
Replacement: Node's built-in
util.parseEnv()(cli-kit already requiresengines.node ≥ 22.12).packages/cli-kit/src/public/node/dot-env.ts—dotenv.parse()→util.parseEnv().@types/nodebump (18.19.70→22.19.17): The pinned types predatedutil.parseEnv, so a naked import wouldn't type-check. Rather than a local type shim, the types are bumped to match theengines.node ≥ 22.12requirement (root +packages/e2e).parseEnvtypes values asstring | undefined, so the result is narrowed toRecord<string, string>at the single call site with a short documented cast. The bump also widenedprocess.exit's signature, so two theme test mocks are updated to match.ESLint rule fix (
no-inline-graphql), separate commit: The@types/nodebump deterministically flips how pnpm installs@shopify/eslint-plugin-cli(file:injection →link:symlink), which shifts the rule's__dirnameand broke its fixed-offset repo-root calculation — flagging every grandfathered file. The repo-root lookup is now install-layout-independent: it asks git (git rev-parse --show-toplevel) instead of deriving the root from__dirname. Kept as its own commit so it lands cleanly under rebase-merge.Validation: CI green —
type-check,lint,vitest,e2e.Out of scope: The e2e package keeps its own
dotenvdependency (only its@types/nodewas bumped here).🤖 AI-generated draft — needs human review before merge.