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

Environment variables break using vite's intlayerPlugin #91

Open
seaders opened this issue Jan 27, 2025 · 12 comments
Open

Environment variables break using vite's intlayerPlugin #91

seaders opened this issue Jan 27, 2025 · 12 comments
Assignees

Comments

@seaders
Copy link

seaders commented Jan 27, 2025

Sample app: https://github.com/SuperStreaming/intlayerapp

Stackblitz: https://stackblitz.com/~/github.com/SuperStreaming/intlayerapp (this unfortunately doesn't work due to a import { findMatchingCondition } from "../../index.mjs"; in getEnumerationContent.mjs:1 which you might want to take a look at too)

Basically, in this project, there is a .env.development and a .env.development.local env vars. Before using intlayerPlugin with vite, using import.meta.env.VITE_MY_ENV_VAR in my code correctly shows the .local content, "seaders2". After, it incorrectly shows seaders.

It also seems to some very strange things to my environment overall. I had a situation where production vars (from .env.production) were "leaking" into a build for dev, as well as if I changed variables in run-time they weren't reflected, sometimes even after restarting the server fully, sometimes I even had to reload VS Code?

I know for a fact that

will cause some of these ill effects as, due to running into a similar issue previously, I know that mutating process.env directly is a no-no, and causes vite's internal processing of environment variables to fall over in weird ways. I know this because I hit this issue previously with vite -

vitejs/vite#18993 (comment)

Unfortunately just removing those lines didn't resolve it. I ran a local copy of the plugin, without that env manipulation, and the error persisted.

I suspect the loadEnvFile probably needs changing too, but I can't see where this is actually used https://github.com/aymericzip/intlayer/blob/main/packages/%40intlayer/config/src/envVariables/loadEnvFile.ts I also see where an Object.assign(process.env is happening here,

Object.assign(process.env, env);
but I can't see that happening in the actual code.

@aymericzip aymericzip self-assigned this Jan 27, 2025
@aymericzip
Copy link
Owner

aymericzip commented Jan 27, 2025

Hey Seaders,

Thanks again for your Issue! Thank you also for all the details provided and for taking the time to publish an example on StackBlitz.

I fixed the findMatchingCondition path. I will release the fix in the next hour (v4.1.5)

I will take the time to investigate the environment issue. I will keep you informed about how it goes.

@aymericzip
Copy link
Owner

aymericzip commented Jan 27, 2025

I have patched the bug related to the environment variables (v4.1.6).

Thank you for your investigation! Your reasoning was very accurate.


Detail:

Indeed, Intlayer injects environment variables into the Vite configuration. The injected variables are only related to Intlayer's configuration. They allow retrieving the configuration state throughout the entire project.

Therefore, Intlayer must read the configuration file's state and load the environment variables to interpret it.

Example of intlayer.config.ts file including environment variables:

import { type IntlayerConfig } from 'intlayer';

const config: IntlayerConfig = {
  editor: {
    applicationURL: process.env.VITE_INTLAYER_APPLICATION_URL,
  },
};

export default config;

And indeed, loadEnvFile loads the environment variables using dotenv.

I would have thought that Vite would overwrite these variables, but it does not. Loading variables for the Intlayer's config purpose injects these variables into Vite's environment.

Additionally, the priority in loading the dotenv files was incorrect:

const env = process.env.NODE_ENV 

const loadEnvFile = () => 
  dotenv.config({ path: ['.env', `.env.${env}`, `.env.${env}.local`] });

// Instead of:

const loadEnvFile = () => 
  dotenv.config({ path: [`.env.${env}.local`, `.env.${env}`, '.env'] });

Please let me know if it works as expected 😊

@seaders
Copy link
Author

seaders commented Jan 27, 2025

I absolutely will do, am just annoyed I didn't spot that ordering issue!

Any reason you're using dotenv stuff here, and not vite's loadEnv? https://vite.dev/guide/api-javascript.html#loadenv

That loading can (does) still mutate the env. The advice that was given to me was to also pass processEnv: {} there, that's what the vite guys suggested vitejs/vite#18993 (comment)

@seaders
Copy link
Author

seaders commented Jan 27, 2025

Just tried it there @aymericzip and I can confirm the initial issue is resolved, but "hot" reloading of envs isn't working. I'm 99% sure that it's due to the above. Config is mutating env and breaking vite's server's connection to it. Bonkers situation, but that's what it does.

If you went this way, you could then also remove the dependency on dotenv from the backend and config packages.

@aymericzip
Copy link
Owner

Indeed, it would be possible to use loadEnv to load these environment variables. However, I structured my application so that a package (@intlayer/config) handles everything related to Intlayer's configuration. This package is then called in other packages, working with Webpack (React CRA/NextJS), as well as the CLI or Express.js. Vite was the most recent environment implemented. Using dotenv therefore seemed more appropriate.

I just noticed, by the way, that Vite also relies on dotenv for loadEnv.

Thank you for raising these questions, as I just realized that .env.local was missing from the list of files.

And thanks again for reporting the hot reloading issue. I'll take a closer look at it.

@seaders
Copy link
Author

seaders commented Jan 28, 2025

Ahhhh sorry, wasn't thinking properly, that makes sense.

Vite also relies on dotenv for loadEnv.

Yep, but the issue is doing that multiple times. As per dotenv's docs,
https://github.com/motdotla/dotenv?tab=readme-ov-file#config

config will read your .env file, parse the contents, assign it to process.env, and return an Object with a parsed key containing the loaded content or an error key if it failed.

The thing is though, am I right in saying that we shouldn't even be getting in to here?

Per https://github.com/aymericzip/intlayer/blob/main/packages/%40intlayer/config/src/configFile/loadConfigurationFile.ts#L11

Image

getPlatform() here shouldn't be undefined for me right, but is? Does that mean stuff's working a bit oddly here? We jumped to vite 6 a while ago, might that have messed with things?

@aymericzip
Copy link
Owner

Once again, you're absolutely right. You raise some very valid points.

Logically speaking, getPlatform() always returns unknown because, when reading the configuration file, we are not in the application's runtime environment (client-side). Therefore, it is not possible for getPlatform() to return values such as "vite", "next", or "react_app".

Additionally, I haven't found a solution to avoid overwriting the environment variables.

As such, it seems we should remove the loadEnvFile() function from the file that includes loadConfigurationFile(). However, I’m concerned that this might break the functionality of the CLI as well as the Next.js and Webpack plugins.

Here’s the link to my latest commit, which includes some minor changes. However, it does not resolve the issue mentioned:
af7e69d

@seaders
Copy link
Author

seaders commented Jan 28, 2025

Just to make sure you're aware, I've raised this issue with vite here - vitejs/vite#19306 as again, I consider this to be an issue with them. I'm lucky that you've opensourced this, and you've been so responsive, but if either of those things weren't true, I may not know the cause of this issue at all (if closed source), or not have any solution at all.

But, for the changes, without passing in an obj to dotenv.config, it still manipulates process.env.

This prevents that (and would have prevented the OG, initial issue),

export const loadEnvFile = () => {
  const result = {}
  dotenv.config({
    path: [`.env.${env}.local`, `.env.${env}`, ".env.local", ".env"],
    processEnv: result
  })
  return result
}

Because this mutation still happens in the plugin

    process.env = {
      ...process.env, // Env var eventually provided by other plugins
      ...viteEnvVar, // Env var loaded by vite .env files
      ...intlayerEnvVar, // Env var related to intlayer
    };

I think (though not sure without testing) the env vars setup with vite will still fall over.

@aymericzip
Copy link
Owner

Amazing!
Thank you once again for your suggestion.

I’ve implemented the change, and it seems to be working well:
4c38ea2

@seaders
Copy link
Author

seaders commented Jan 28, 2025

That looks awesome!

FYI I just checked and the process.env mutation doesn't break things.

@seaders
Copy link
Author

seaders commented Jan 29, 2025

Are these changes in 4.1.7 @aymericzip ? I saw that that version bump, and push to npm went before 4c38ea2 . I've just installed 4.1.7 and the problem is still present (but that makes sense if your fix isn't in yet).

@aymericzip
Copy link
Owner

Exactly, I just released it with v4.1.8

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

No branches or pull requests

2 participants