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

Use Vite's envDir option if specified #22

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Conversation

hailwood
Copy link
Contributor

Vite allows you to pass through a custom option for the envDir.
This is useful in cases where you might have multiple frontend packages and so your vite.config.js is not in the root directory.

This update checks for that config option and if so will use it over the current directory when resolving the APP_URL and laravel version.

Vite allows you to pass through a custom option for the envDir.
This is useful in cases where you might have multiple frontend packages and so your vite.config.js is not in the root directory.

This update checks for that config option and if so will use it over the current directory when resolving the APP_URL and laravel version.
@jessarcher
Copy link
Member

Hi @hailwood,

I agree that it makes sense to use envDir when looking for .env files, but I'm not sure that it should also be used to locate composer.json.

It's possible in Laravel to specify a different directory for .env files, so there's no guarantee that the envDir and the Laravel root directory are the same.

The loading of composer.json is already in a try/catch, so the worst-case scenario should be that no version information is displayed.

Can we leave the laravelVersion() as-is, and just use envDir to load the APP_URL env var?

@hailwood
Copy link
Contributor Author

Hi @jessarcher,
You are correct of course that you can change where the .env files are in laravel, but since they're both edge cases I wonder what's the most common -

  1. vite.config.js file not in root
  2. .env file not located next to the composer.json

Happy to revert the laravelVersion change if you believe edge case 2 is more common than edge case 1.
Let me know :)

@jessarcher
Copy link
Member

Thanks, @hailwood. I agree that the first edge case seems more common, but haven't personally run into either.

It's mostly that I'm just not a fan of using envDir to find composer.json. We could add a config option to the plugin for the "laravel root", but it doesn't seem worthwhile just to display the version number. The plugin could also recursively look up the directory tree until it finds a composer.json, but again that seems like overkill to grab a version number. I think I'd almost rather just not display the Laravel version number, or display the plugin version number instead.

If you wouldn't mind reverting the laravelVersion() change I'll get this merged in, and perhaps we can come up with another solution for the Laravel version later.

@jessarcher
Copy link
Member

Thanks, @hailwood!

@jessarcher jessarcher merged commit d4fc875 into laravel:main Jun 22, 2022
@hailwood
Copy link
Contributor Author

No worries :)
Thanks for all your work on this!

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