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

fix(build): NODE_ENV override by .env #6303

Merged
merged 2 commits into from
Jan 26, 2022
Merged

fix(build): NODE_ENV override by .env #6303

merged 2 commits into from
Jan 26, 2022

Conversation

RainKolwa
Copy link
Contributor

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy
Copy link
Member

bluwy commented Dec 30, 2021

Fixes #6302

@Niputi Niputi linked an issue Dec 30, 2021 that may be closed by this pull request
7 tasks
@bluwy
Copy link
Member

bluwy commented Dec 30, 2021

This and #6304 are solving the same issue. But I think #6304 is more correct as currently .env has higher priority over .env.mode. This PR switches the priority for NODE_ENV only, which would be inconsistent.

I do however think that Vite is sorting the priority in reverse than expected though. But fixing it would be a breaking change.

@patak-dev
Copy link
Member

@bluwy would you like to PR reverting the priority? We can discuss it next week (Friday), and maybe include it in 2.8? It may be too big of a change for a minor and we may need to leave it for 3.0 in May/June

@poyoho
Copy link
Member

poyoho commented Dec 30, 2021

If we want to change I still think this should be changed 😀. Should be caused by the order here ?

@Shinigami92
Copy link
Member

If we want to change I still think this should be changed grinning. Should be caused by the order here ?

Why? If I understand line

env[key] === undefined

correctly, then it first use the most specific file and defines the vars.
If another file gets to this same var, it will not override it anymore 🤔 and IMO this is exactly what we want, do we?

@userquin
Copy link
Contributor

userquin commented Dec 31, 2021

check https://cli.vuejs.org/guide/mode-and-env.html#environment-variables, see green alert with title Env Loading Priorities, vite works as expected, we can add this green hint also on vite docs (added this comment also on #6304 )

https://discord.com/channels/804011606160703521/804011606160703524/926173658219708416

@poyoho
Copy link
Member

poyoho commented Dec 31, 2021

you are right! 👍

@bluwy
Copy link
Member

bluwy commented Dec 31, 2021

Uhhh I should've tested the .env priority locally. Looks like the priority is indeed working properly and only NODE_ENV wasn't handled right. We should keep this PR then. My bad!

@bluwy
Copy link
Member

bluwy commented Jan 26, 2022

Would be great to get this merged to fix the issue. The changes looks good to me.

@patak-dev patak-dev merged commit 7329b24 into vitejs:main Jan 26, 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.

import.meta.env.MODE=production while import.meta.env.PROD returned false
6 participants