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: add option to exclude global define and env replacement #5577

Closed
wants to merge 2 commits into from

Conversation

ygj6
Copy link
Member

@ygj6 ygj6 commented Nov 7, 2021

Description

Intent to fix vuejs/vitepress#419, but not completely resolved.

Ideally, the replacement of define and env can be turned off by specifying options, as in my test example.
I put forward this PR to stimulate discussion about whether the problems caused by string replacement should be fixed.
This is just one of my attempts, there should be other better ways to welcome you to point it out.
Thank you for your positive opinions.

Additional context

When I wrote the test example, I also found the error of define and pre-building, see also: #5570


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.

@ygj6 ygj6 marked this pull request as draft November 7, 2021 09:57
@ygj6 ygj6 marked this pull request as ready for review November 9, 2021 02:30
@ygj6 ygj6 force-pushed the env-replacement branch 2 times, most recently from 3a81141 to 352714c Compare November 16, 2021 12:30
packages/playground/define/__tests__/define.spec.ts Outdated Show resolved Hide resolved
packages/playground/define/index.html Outdated Show resolved Hide resolved
packages/playground/define/vite.config.js Outdated Show resolved Hide resolved
packages/playground/define/vite.config.js Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/define.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added p2-nice-to-have Not breaking anything but nice to have (priority) YAO Yet another option... labels Nov 16, 2021
@kiaking
Copy link
Contributor

kiaking commented May 24, 2022

@patak-dev Would it be possible to have this merged? Or maybe I might need a bit help on this issue. It's related to vuejs/vitepress#419 and it's breaking Vite 3 docs page as well (Env Variables and Modes page) 😅

@patak-dev
Copy link
Member

@kiaking we need to discuss this with the team, but I don't think it is the best option. A user may want to use import.meta.env in their .md inside Vue components, and will have the same problem. Maybe we need some flag so Vite would ignore these, like \0process.env.
We have a few instances in the Vite docs, and we are now doing this as a workaround until we find a good path forward
https://github.com/vitejs/vite/blob/main/docs/guide/ssr.md?plain=1#L172

@kiaking
Copy link
Contributor

kiaking commented May 24, 2022

Ah, I see good to see the workaround. OK, in that case we can skip this issue targeting VitePress alpha and just workaround with that technique for now. We can relax a bit then 👍

@bluwy
Copy link
Member

bluwy commented Apr 1, 2023

I think we should find a more robust solution than a new config option to conditionally replace in some files. Maybe #11151 would help with this. IIRC there's also a builtin workaround for this in VitePress. Closing for now, but thanks for kicking off this option!

@bluwy bluwy closed this Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority) YAO Yet another option...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

pages contains process.env.NODE_ENV !== 'production' report 404 error
5 participants