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

Ampli CLI API_URL and APP_ENV env var conflicts #600

Open
aweber1 opened this issue Oct 13, 2023 · 2 comments
Open

Ampli CLI API_URL and APP_ENV env var conflicts #600

aweber1 opened this issue Oct 13, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@aweber1
Copy link

aweber1 commented Oct 13, 2023

Since the @amplitude/ampli CLI library is not on GitHub (why?), I'm opening this issue here.

The ampli CLI for some reason references environment variables in emitted code and unfortunately at least three of the env vars it references are named too generically: process.env.API_URL, process.env.APP_ENV, process.env.APP_URL. I would highly suggest changing those env vars to be scoped to the ampli library, e.g. AMPLI_CLI_API_URL, AMPLI_CLI_APP_ENV, AMPLI_CLI_APP_URL or similar. OR, better yet, don't ship package code that reads env vars - specifically to avoid issues like this. Instead, always make config injectable.

Version: @amplitude/ampli 1.34.0

@aweber1 aweber1 added the bug Something isn't working label Oct 13, 2023
@aweber1
Copy link
Author

aweber1 commented Oct 14, 2023

If you happen to be using patch-package, here is a patch to fix this issue until (if) it is resolved:

file name: @amplitude+ampli+1.34.0.patch

diff --git a/node_modules/@amplitude/ampli/lib/constants.js b/node_modules/@amplitude/ampli/lib/constants.js
index 0d7c7a6..4852c93 100644
--- a/node_modules/@amplitude/ampli/lib/constants.js
+++ b/node_modules/@amplitude/ampli/lib/constants.js
@@ -4,7 +4,7 @@ exports.OAuthRefreshTokenTTLHours = exports.OAuthAccessTokenTTLHours = exports.A
 const { version } = require('../package.json');
 const sentryEnabled = process.env.AMPLI_SENTRY !== 'disabled';
 const sentryDSN = sentryEnabled ? 'https://f2587c0897834285be5dab446d5387db@o13027.ingest.sentry.io/5974462' : undefined;
-const environment = process.env.APP_ENV || 'production';
+const environment = 'production';
 exports.ZONE_SETTINGS = {
     us: {
         apiUrl: 'https://data-api.amplitude.com/graphql',
@@ -20,14 +20,14 @@ exports.ZONE_SETTINGS = {
     },
 };
 exports.APP_SETTINGS = Object.freeze({
-    localDevelopment: process.env.LOCAL_DEVELOPMENT === 'true',
+    localDevelopment: false,
     app: {
         environment,
-        version: process.env.AMPLI_VERSION || version,
+        version,
     },
     ampli: (zone) => ({
-        apiUrl: (process.env.API_URL || exports.ZONE_SETTINGS[zone].apiUrl).replace(/\/+$/, ''),
-        webUrl: (process.env.APP_URL || exports.ZONE_SETTINGS[zone].webUrl).replace(/\/+$/, ''),
+        apiUrl: (exports.ZONE_SETTINGS[zone].apiUrl).replace(/\/+$/, ''),
+        webUrl: (exports.ZONE_SETTINGS[zone].webUrl).replace(/\/+$/, ''),
     }),
     sentry: {
         enabled: sentryEnabled,


@qingzhuozhen
Copy link
Contributor

Hi @aweber1, thanks for reporting this and sharing the fix patch. We will look further into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants