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.

Sign up for GitHub

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

Reload environment variables in development #997

Merged
merged 8 commits into from
Jul 18, 2023
+207 −128
Merged

Reload environment variables in development #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c25342f
Load env vars in parallel to initial build; Do not throw on env var e…
frandiox Jun 9, 2023
9eafba8
Reload env variables when project is linked to storefront
frandiox Jun 9, 2023
2a03c45
Wrap all calls in try-catch
frandiox Jun 9, 2023
5753dbf
Merge branch '2023-04' into fd-reload-env-dev
frandiox Jul 6, 2023
e8e2c00
Resolve variables in parallel and avoid crashing on fetch fail
frandiox Jul 6, 2023
eebb29f
Add .env to Remix watcher in templates
frandiox Jul 6, 2023
812368e
Changesets
frandiox Jul 6, 2023
7816a4a
Merge branch '2023-04' into fd-reload-env-dev
frandiox Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .changeset/smart-cooks-divide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'demo-store': patch
---

Add `.env` file to Remix watcher to allow reloading environment variables on file save. In `remix.config.js`:

```diff
-watchPaths: ['./public'],
+watchPaths: ['./public', './.env'],
```
5 changes: 5 additions & 0 deletions .changeset/tricky-pears-dress.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/cli-hydrogen': minor
---

Reload environment variables in the development server when `.env` file is updated. Show injected variables when project is not linked to any storefront.
16 changes: 8 additions & 8 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"@remix-run/dev": "1.17.1",
"@shopify/cli-kit": "3.47.0",
"@shopify/hydrogen-codegen": "^0.0.2",
"@shopify/mini-oxygen": "^1.6.0",
"@shopify/mini-oxygen": "^1.7.0",
"ansi-escapes": "^6.2.0",
"diff": "^5.1.0",
"fast-glob": "^3.2.12",
Expand Down
45 changes: 27 additions & 18 deletions packages/cli/src/commands/hydrogen/dev.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to do with the logs. Right now it logs "Injecting variables to MiniOxygen: ....." every time the .env file is updated, which is not incorrect but might be too much output. Perhaps we could diff and only show the stuff that has changed.

I've bee going back and forth on where logs should go. Maybe it'd be easier if certain components were broken down even further into functions so that things could be more reuseable?

On save I think it'd be better to have something like "Change detected; reloading environment variables". I like the idea of a diff to shorten the output but might be more work than it's worth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often will someone realistically be changing the .env file? If I change it, I think it's nice to know that the server is getting those changes. So I think the feedback is good and I don't imagine it changing all that often.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, keeping the same logs for now 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, right now this is downloading remote vars again on file save... Perhaps we should assume the remote variables don't change and we only read the local .env on file save?

I actually kind of love that it fetches them again but I wonder if that would be considered unexpected...

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'path';
import fs from 'fs/promises';
import {outputDebug, outputInfo} from '@shopify/cli-kit/node/output';
import {fileExists} from '@shopify/cli-kit/node/fs';
import {renderFatalError} from '@shopify/cli-kit/node/ui';
import {renderFatalError, renderWarning} from '@shopify/cli-kit/node/ui';
import colors from '@shopify/cli-kit/node/colors';
import {copyPublicFiles} from './build.js';
import {
Expand All @@ -14,11 +14,11 @@ import {muteDevLogs, warnOnce} from '../../lib/log.js';
import {deprecated, commonFlags, flagsToCamelObject} from '../../lib/flags.js';
import Command from '@shopify/cli-kit/node/base-command';
import {Flags} from '@oclif/core';
import {startMiniOxygen} from '../../lib/mini-oxygen.js';
import {type MiniOxygen, startMiniOxygen} from '../../lib/mini-oxygen.js';
import {checkHydrogenVersion} from '../../lib/check-version.js';
import {addVirtualRoutes} from '../../lib/virtual-routes.js';
import {spawnCodegenProcess} from '../../lib/codegen.js';
import {combinedEnvironmentVariables} from '../../lib/combined-environment-variables.js';
import {getAllEnvironmentVariables} from '../../lib/environment-variables.js';
import {getConfig} from '../../lib/shopify-config.js';

const LOG_REBUILDING = '🧱 Rebuilding...';
Expand Down Expand Up @@ -121,10 +121,8 @@ async function runDev({
const serverBundleExists = () => fileExists(buildPathWorkerFile);

const {shop, storefront} = await getConfig(root);
const environmentVariables =
!!shop && !!storefront?.id
? await combinedEnvironmentVariables({root, shop, envBranch})
: undefined;
const fetchRemote = !!shop && !!storefront?.id;
const envPromise = getAllEnvironmentVariables({root, fetchRemote, envBranch});

const [{watch}, {createFileWatchCache}] = await Promise.all([
import('@remix-run/dev/dist/compiler/watch.js'),
Expand All @@ -135,21 +133,19 @@ async function runDev({
let initialBuildDurationMs = 0;
let initialBuildStartTimeMs = Date.now();

let isMiniOxygenStarted = false;
let miniOxygen: MiniOxygen;
async function safeStartMiniOxygen() {
if (isMiniOxygenStarted) return;
if (miniOxygen) return;

const miniOxygen = await startMiniOxygen({
miniOxygen = await startMiniOxygen({
root,
port,
watch: true,
buildPathWorkerFile,
buildPathClient,
environmentVariables,
env: await envPromise,
});

isMiniOxygenStarted = true;

miniOxygen.showBanner({
appName: storefront ? colors.cyan(storefront?.title) : undefined,
headlinePrefix:
Expand All @@ -174,6 +170,7 @@ async function runDev({
}

const fileWatchCache = createFileWatchCache();
let skipRebuildLogs = false;

await watch(
{
Expand All @@ -188,22 +185,23 @@ async function runDev({
{
reloadConfig,
onBuildStart() {
if (!isInitialBuild) {
console.time(LOG_REBUILT);
if (!isInitialBuild && !skipRebuildLogs) {
outputInfo(LOG_REBUILDING);
console.time(LOG_REBUILT);
}
},
async onBuildFinish() {
if (isInitialBuild) {
await copyingFiles;
initialBuildDurationMs = Date.now() - initialBuildStartTimeMs;
isInitialBuild = false;
} else {
} else if (!skipRebuildLogs) {
skipRebuildLogs = false;
console.timeEnd(LOG_REBUILT);
if (!isMiniOxygenStarted) console.log(''); // New line
if (!miniOxygen) console.log(''); // New line
}

if (!isMiniOxygenStarted) {
if (!miniOxygen) {
if (!(await serverBundleExists())) {
return renderFatalError({
name: 'BuildError',
Expand Down Expand Up @@ -235,6 +233,17 @@ async function runDev({
const [relative, absolute] = getFilePaths(file);
outputInfo(`\n📄 File changed: ${relative}`);

if (relative.endsWith('.env')) {
skipRebuildLogs = true;
await miniOxygen.reload({
env: await getAllEnvironmentVariables({
root,
fetchRemote,
envBranch,
}),
});
}

if (absolute.startsWith(publicPath)) {
await copyPublicFiles(
absolute,
Expand Down
76 changes: 0 additions & 76 deletions packages/cli/src/lib/combined-environment-variables.ts
View file
Open in desktop

This file was deleted.

44 changes: 35 additions & 9 deletions ...ib/combined-environment-variables.test.ts → ...cli/src/lib/environment-variables.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs';
import {joinPath} from '@shopify/cli-kit/node/path';
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output';

import {combinedEnvironmentVariables} from './combined-environment-variables.js';
import {getAllEnvironmentVariables} from './environment-variables.js';
import {getStorefrontEnvVariables} from './graphql/admin/pull-variables.js';
import {login} from './auth.js';

vi.mock('./auth.js');
vi.mock('./graphql/admin/pull-variables.js');

describe('combinedEnvironmentVariables()', () => {
describe('getAllEnvironmentVariables()', () => {
const ADMIN_SESSION = {
token: 'abc123',
storeFqdn: 'my-shop',
Expand Down Expand Up @@ -52,10 +52,9 @@ describe('combinedEnvironmentVariables()', () => {

it('calls pullRemoteEnvironmentVariables', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await combinedEnvironmentVariables({
await getAllEnvironmentVariables({
envBranch: 'main',
root: tmpDir,
shop: 'my-shop',
});

expect(getStorefrontEnvVariables).toHaveBeenCalledWith(
Expand All @@ -66,11 +65,23 @@ describe('combinedEnvironmentVariables()', () => {
});
});

it('does not call pullRemoteEnvironmentVariables when indicated', async () => {
await inTemporaryDirectory(async (tmpDir) => {
await getAllEnvironmentVariables({
envBranch: 'main',
root: tmpDir,
fetchRemote: false,
});

expect(getStorefrontEnvVariables).not.toHaveBeenCalled();
});
});

it('renders a message about injection', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outputMock = mockAndCaptureOutput();

await combinedEnvironmentVariables({root: tmpDir, shop: 'my-shop'});
await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).toMatch(
/Environment variables injected into MiniOxygen:/,
Expand All @@ -82,12 +93,27 @@ describe('combinedEnvironmentVariables()', () => {
await inTemporaryDirectory(async (tmpDir) => {
const outputMock = mockAndCaptureOutput();

await combinedEnvironmentVariables({root: tmpDir, shop: 'my-shop'});
await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).toMatch(/PUBLIC_API_TOKEN\s+from Oxygen/);
});
});

it('doest not fail on network errors', async () => {
await inTemporaryDirectory(async (tmpDir) => {
vi.mocked(getStorefrontEnvVariables).mockRejectedValue(
new Error('Network error'),
);

const outputMock = mockAndCaptureOutput();

await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).not.toMatch(/PUBLIC_API_TOKEN\s+from Oxygen/);
expect(outputMock.warn()).toMatch(/failed to load/i);
});
});

describe('when one of the variables is a secret', () => {
beforeEach(() => {
vi.mocked(getStorefrontEnvVariables).mockResolvedValue({
Expand All @@ -107,7 +133,7 @@ describe('combinedEnvironmentVariables()', () => {
await inTemporaryDirectory(async (tmpDir) => {
const outputMock = mockAndCaptureOutput();

await combinedEnvironmentVariables({root: tmpDir, shop: 'my-shop'});
await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).toMatch(
/PUBLIC_API_TOKEN\s+from Oxygen \(Marked as secret\)/,
Expand All @@ -124,7 +150,7 @@ describe('combinedEnvironmentVariables()', () => {

const outputMock = mockAndCaptureOutput();

await combinedEnvironmentVariables({root: tmpDir, shop: 'my-shop'});
await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).toMatch(/LOCAL_TOKEN\s+from local \.env/);
});
Expand All @@ -138,7 +164,7 @@ describe('combinedEnvironmentVariables()', () => {

const outputMock = mockAndCaptureOutput();

await combinedEnvironmentVariables({root: tmpDir, shop: 'my-shop'});
await getAllEnvironmentVariables({root: tmpDir});

expect(outputMock.info()).not.toMatch(
/PUBLIC_API_TOKEN\s+from Oxygen/,
Expand Down
Loading