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

feat(env): support dotenv-expand to contains process env #10370

Merged
merged 11 commits into from
Nov 8, 2022

Conversation

Dunqing
Copy link
Contributor

@Dunqing Dunqing commented Oct 7, 2022

Description

close: #6626

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 bluwy added breaking change p3-significant High priority enhancement (priority) labels Oct 7, 2022
@bluwy bluwy added this to the 4.0 milestone Oct 7, 2022
@bluwy
Copy link
Member

bluwy commented Oct 7, 2022

Marking this as a breaking change and for 4.0 since there were regression reports when we last tried to update, but thanks for starting work on this! We could be merging this once 4.0 is near. (when the test is fixed too 😬 )

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 7, 2022

Hope version 4 will come soon😁

@bluwy
Copy link
Member

bluwy commented Oct 9, 2022

Vite 4 will follow shortly after Rollup 3 is released, which is soon I think 👍

@benmccann
Copy link
Collaborator

@Dunqing main is now targeting Vite 4. Would you be able to update this PR to address the merge conflict?

playground/env/index.html Outdated Show resolved Hide resolved
patak-dev
patak-dev previously approved these changes Nov 8, 2022
@sapphi-red
Copy link
Member

I think bumping dotenv-expand can be done independently without this change because we're not using the change in 9.0.0.

Also dotenv-expand requires dotenv v16. (It doesn't include peerDep though.) So I think we should upgrade it together if we are upgrading dotenv-expand.
https://github.com/motdotla/dotenv-expand/blob/master/CHANGELOG.md#800-2022-02-03

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

The code itself looks good to me.

packages/vite/src/node/env.ts Show resolved Hide resolved
bluwy
bluwy previously approved these changes Nov 8, 2022
@Dunqing
Copy link
Contributor Author

Dunqing commented Nov 8, 2022

I think bumping dotenv-expand can be done independently without this change because we're not using the change in 9.0.0.

Yes

Also dotenv-expand requires dotenv v16. (It doesn't include peerDep though.) So I think we should upgrade it together if we are upgrading dotenv-expand. https://github.com/motdotla/dotenv-expand/blob/master/CHANGELOG.md#800-2022-02-03

I can make a new PR to support #10149, or do it here and close #10149, what do you think?

@sapphi-red
Copy link
Member

I can make a new PR to support #10149, or do it here and close #10149, what do you think?

IMHO it would be better to remove bumping dotenv-expand from this PR and splitted into a PR that includes upgrading dotenv.

@sapphi-red
Copy link
Member

Would you revert the pnpm.lock change just in case?

@sapphi-red
Copy link
Member

Thanks for updating!


I just noticed that it throws an error with dotenv-expand@>=6.0.0 if a user had an env var that includes $.
The error is TypeError: Cannot read properties of undefined (reading 'split') that is same with #6858.

With powershell, you can reproduce by running $env:VITE_FOO="$`f"; pnpm dev. With bash, I guess $VITE_FOO="\$f" pnpm dev will do the same thing.

This is not a direct blocker of this PR, but this PR will block #10826 if merged.

Maybe we should wait for dotenv-expand to fix that. Or we could make a PR to dotenv-expand and apply that patch with pnpm patch for a while (It works because we are bundling dotenv-expand).

@Dunqing
Copy link
Contributor Author

Dunqing commented Nov 8, 2022

This is not a direct blocker of this PR, but this PR will block #10826 if merged.

Maybe we should wait for dotenv-expand to fix that. Or we could make a PR to dotenv-expand and apply that patch with pnpm patch for a while (It works because we are bundling dotenv-expand).

Ok, I see, I think we can fix the problem with dotenv-expand in #10826, instead of blocking this PR.

@patak-dev patak-dev merged commit d5fe92c into vitejs:main Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exposed env vars are not available
6 participants