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

DRAFT: Allow reloading of env variables before restarting nuxt dev server #466

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abhi12299
Copy link

@abhi12299 abhi12299 commented Sep 3, 2024

πŸ”— Linked issue

Refer to nuxt/nuxt#28543 to reproduce this issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When the contents of .env change, the dev server is unable to pick up the changes, even after the server restarts.

There are multiple ways to go about implementing this feature, one of which is demonstrated in this PR. c12 library is used to setup and load environment variables from .env file. The following code is responsible for the same:

cli/src/commands/dev.ts

Lines 47 to 52 in 34bdbbc

async run(ctx) {
// Prepare
overrideEnv('development')
const cwd = resolve(ctx.args.cwd || ctx.args.rootDir || '.')
showVersions(cwd)
await setupDotenv({ cwd, fileName: ctx.args.dotenv })

The function responsible for restarting the child process is:

cli/src/commands/dev.ts

Lines 167 to 187 in 34bdbbc

const restart = async () => {
// Kill previous process with restart signal
kill('SIGHUP')
// Start new process
childProc = fork(globalThis.__nuxt_cli__!.entry!, ['_dev', ...rawArgs], {
execArgv: [
'--enable-source-maps',
process.argv.includes('--inspect') && '--inspect',
].filter(Boolean) as string[],
env: {
...process.env,
__NUXT_DEV__: JSON.stringify({
proxy: {
url: devProxy.listener.url,
urls: await devProxy.listener.getURLs(),
https: devProxy.listener.https,
},
} satisfies NuxtDevContext),
},
})

You'll notice that process.env is expanded directly, and there is no step to reload the environment variables from the changed .env file.

The change I proposed keeps track of the cwd and dotenv args passed to the dev command and then run this code prior to forking a new process:

if (_cwd) {
    const env = await loadDotenv({ cwd: _cwd, fileName: _dotenv || '.env', interpolate: true })
    Object.assign(process.env, env)
}

The reason for not using the same setupDotenv function provided by c12 is because if you look at the definition of the function (taken from c12's repo):

export async function setupDotenv(options: DotenvOptions): Promise<Env> {
  const targetEnvironment = options.env ?? process.env;

  // Load env
  const environment = await loadDotenv({
    cwd: options.cwd,
    fileName: options.fileName ?? ".env",
    env: targetEnvironment,
    interpolate: options.interpolate ?? true,
  });

  // Fill process.env
  for (const key in environment) {
    if (!key.startsWith("_") && targetEnvironment[key] === undefined) { // -> ***notice this line
      targetEnvironment[key] = environment[key];
    }
  }

  return environment;
}

/** Load environment variables into an object. */
export async function loadDotenv(options: DotenvOptions): Promise<Env> {
  const environment = Object.create(null);

  const dotenvFile = resolve(options.cwd, options.fileName!);

  if (existsSync(dotenvFile)) {
    const parsed = dotenv.parse(await fsp.readFile(dotenvFile, "utf8"));
    Object.assign(environment, parsed);
  }

  // Apply process.env
  if (!options.env?._applied) {
    Object.assign(environment, options.env);
    environment._applied = true;
  }

  // Interpolate env
  if (options.interpolate) {
    interpolate(environment);
  }

  return environment;
}

Assume that an environment variable NUXT_API_KEY gets changed. Now the line marked with *** will check if the key is undefined in the target environment, i.e. process.env, which in this case is actually defined. So it will never overwrite the value.

I've marked this PR as draft because we can instead request upstream changes to c12 to tidy up the code here.

@abhi12299 abhi12299 marked this pull request as draft September 3, 2024 11:19
@abhi12299
Copy link
Author

@pi0 can you please review this? Would like to know your thoughts.

@abhi12299
Copy link
Author

@danielroe can you please review when you get time. I can work on an upstream fix if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant