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

Allow chosing different env files #113

Merged
merged 3 commits into from
Aug 8, 2022
Merged

Allow chosing different env files #113

merged 3 commits into from
Aug 8, 2022

Conversation

LucdoOf
Copy link
Contributor

@LucdoOf LucdoOf commented Aug 3, 2022

Hello,

I have a project in which i need to work with several env files, and none of them are simply called .env.

I usually chose my env file using the mode option of vite.

But, when you run a vite server using mode option on an existing env file without a default .env file the server crash with this error:

/var/www/Boreal/Upcover/Instances/Upcovel/node_modules/laravel-vite-plugin/dist/index.js:110
                        server.config.logger.info(`  ${picocolors_1.default.green('➜')}  ${picocolors_1.default.bold('APP_URL')}: ${picocolors_1.default.cyan(appUrl.replace(/:(\d+)/, (_, port) => `:${picocolors_1.default.bold(port)}`))}`);
                                                                                                                                                                     ^

TypeError: Cannot read properties of undefined (reading 'replace')
    at Timeout._onTimeout (/var/www/Boreal/Upcover/Instances/Upcovel/node_modules/laravel-vite-plugin/dist/index.js:110:166)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)

In fact, in the configureServer method the env file loaded is the .env file, and it cannot be changed:

const appUrl = loadEnv('', envDir, 'APP_URL').APP_URL

I suggest to change this line to:

const appUrl = loadEnv(resolvedConfig.mode, envDir, 'APP_URL').APP_URL

And thus, add:

mode: mode,

In the config definition, it fixes the issue.

@LucdoOf LucdoOf changed the title Fixed default mode in configureServer Allow chosing different env files Aug 3, 2022
@jessarcher jessarcher self-requested a review August 4, 2022 06:26
src/index.ts Outdated
@@ -99,6 +99,7 @@ function resolveLaravelPlugin(pluginConfig: Required<PluginConfig>): LaravelPlug
const assetUrl = env.ASSET_URL ?? ''

return {
mode: mode,
Copy link
Member

@jessarcher jessarcher Aug 5, 2022

Choose a reason for hiding this comment

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

Was there a reason for this change? It seems to work fine without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh really? Thought it wasn't defined at all. Maybe i went to quickly! Thanks for the quick review :)

Copy link
Member

Choose a reason for hiding this comment

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

If the user hasn't specified a mode (either via config or the -m flag) then it will be undefined in the UserConfig object at this point. Vite will automatically add the mode to the ResolvedConfig object that we access later, with a default of development, so there's no need for us to manually add it to the UserConfig, unless we needed to change it.

If you can remove this line, I'll get this merged in :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is i don't want vite to parse ".env.development" but my own .env file. If i remove this line and no ".env.development" exists, the same error appears.
Even if i specify a mode using -m, without explicitly adding it to the ResolvedConfig it is set to 'development'.

Copy link
Member

Choose a reason for hiding this comment

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

Specifying mode: mode doesn't really do anything as far as I can tell. The ResolvedConfig object will have the same value with and without it being specified.

The default will always be development with or without it. If you specify a mode in your config or with -m, then the ResolvedConfig config should have that value, even without mode: mode.

If .env.development doesn't exist, that's okay. Vite looks for a few env files and merges the values together: https://github.com/vitejs/vite/blob/60721ac53a1bf326d1cac097f23642faede234ff/packages/vite/src/node/env.ts#L20-L25

Any files that don't exist are silently ignored.

The error you're seeing should only occur if none of the env files contains an APP_URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i understood the confusion.

I was using an npm script (npm run dev --mode=my_env_file) with the corresponding package.json:

"scripts": {
    "dev": "vite",
}

I don't know why, but when a mode is specified this way it doesn't appear in the resolvedConfig object.

Running vite with vite --mode=my_env_file work tho.

I'll just remove the mode:mode line, sorry for this !

Copy link
Member

Choose a reason for hiding this comment

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

I don't think npm run <script> passes arguments or options through to the underlying command, so your -m flag would be ignored.

If you run npm run dev -- -m mode then it should pass everything after the -- through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is it get passed here correctly, even with the npm version:

config: (userConfig, { command, mode }) => {

That's why i'm finding it kinda strange but its ok i'll deal with it :)

Copy link
Member

Choose a reason for hiding this comment

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

That is strange! When I run npm run dev -m test, the mode is passed into this function as development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Strange lol. Let's forget about this haha

@jessarcher
Copy link
Member

This makes sense. Not sure why I didn't do this originally, but I've given it a test using npx vite -m example and it works well 👍

src/index.ts Outdated Show resolved Hide resolved
LucdoOf and others added 2 commits August 8, 2022 13:41
Co-authored-by: Jess Archer <jess@jessarcher.com>
@jessarcher
Copy link
Member

Looks good! Thanks for the contribution!

@jessarcher jessarcher merged commit cfc7d2b into laravel:main Aug 8, 2022
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.

2 participants