-
Notifications
You must be signed in to change notification settings - Fork 317
fix: use esm-env
instead of import.meta.env.MODE
#1770
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
@teemingc is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdded the "esm-env" runtime dependency and switched theme deprecation gating to use the exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant ThemeUtils as ThemeUtils
participant ESM as esm-env
Note over ThemeUtils,ESM #d6f5d6: New runtime flag import
ThemeUtils->>ESM: import { DEV }
ThemeUtils->>ThemeUtils: call warnThemeDeprecation(...)
alt DEV === true
ThemeUtils->>ThemeUtils: emit console warning
else DEV === false
ThemeUtils-->>ThemeUtils: no warning emitted
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
(1 hunks)
"apexcharts": "^5.3.5", | ||
"clsx": "^2.1.1", | ||
"date-fns": "^4.1.0", | ||
"esm-env": "^1.2.2", |
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.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM on adding esm-env
; verify usages are updated and add a changeset
Good call moving away from import.meta.env.MODE
. Please:
- Replace all
import.meta.env.MODE
checks withimport { dev } from 'esm-env'
(orprod/test
as needed). - Add a Changeset (patch) noting the fix for REPL/Playground compatibility.
Run this script to confirm there are no lingering import.meta.env
usages and that esm-env
is actually imported:
Also verify the chosen version is current:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for import.meta.env.MODE"
rg -nP -C2 '\bimport\.meta\.env\.MODE\b' || echo "OK: none found"
echo -e "\n2) Searching for any import.meta.env usage"
rg -nP -C2 '\bimport\.meta\.env\b' || echo "OK: none found"
echo -e "\n3) Confirming esm-env imports exist"
rg -nP -C2 "(?:from|import)\\s+['\"]esm-env['\"]|from\\s+['\"]esm-env['\"]" || echo "WARN: no esm-env imports found"
echo -e "\n4) Check for a pending Changeset"
fd -H -t f '.changeset' -E '.changeset/README.md' | wc -l | awk '{print "changeset files:", $1}'
Length of output: 1020
🌐 Web query:
What is the latest version of the npm package "esm-env"?
💡 Result:
Latest version: 1.2.2. [1][2] (npmjs.com)
Citations:
Replace remaining import.meta.env.MODE
usage with esm-env
and add a changeset
- In
src/lib/theme/themeUtils.ts
(line 6), remove theimport.meta.env.MODE
check and insteadimport { dev } from 'esm-env'
(useprod
/test
as appropriate). - Create a new Changeset (patch) documenting the REPL/Playground compatibility fix.
🤖 Prompt for AI Agents
In package.json (around line 155) and in src/lib/theme/themeUtils.ts (line 6),
replace the runtime check using import.meta.env.MODE with the esm-env export:
remove any import.meta.env.MODE usage and instead import the appropriate flag
from 'esm-env' (e.g., import { dev } from 'esm-env' — or prod/test if that
matches the intended branch) and use that boolean in the existing conditional
logic; then add a new Changeset (patch) describing the REPL/Playground
compatibility fix (one-line summary and bump type patch) so consumers see the
change.
I still get the following error: Could not find dist/ in [email protected] (error occurred while trying to resolve .. within npm://$/[email protected]/dist/theme/themeUtils.js) ![]() |
You're right. The error still occurs https://svelte.dev/playground/598dd47bf2cd4c728dfcae1573d893c6?version=5.35.4 I'll continue to investigate. Thanks. We should probably re-open #1662 |
Closes sveltejs/svelte.dev#1410
Closes #1662
Closes sveltejs/svelte.dev#1536
📑 Description
This PR swaps out the use of
import.meta.env.MODE
withesm-env
. This makes the library work with environments that don't haveimport.meta.env.MODE
.esm-env
is also what SvelteKit uses for dev mode checks.No test because I've no idea how to remove
import.meta.env
when importing the module in Vitest.Status
✅ Checks
api-check
directory as requiredpnpm check && pnpm test:e2e
ℹ Additional Information
Summary by CodeRabbit