fix(html): support `import.meta.env` define replacement without quotes by sapphi-red · Pull Request #13425 · vitejs/vite

sapphi-red · 2023-06-05T09:35:36Z

Description

I'm not sure if this is the best way to solve this.

fixes #13424
refs #12202

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 PR Title 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.

bolt-new-by-stackblitz · 2023-06-05T09:35:38Z

Run & review this pull request in StackBlitz Codeflow.

sapphi-red · 2023-06-05T09:36:18Z

packages/vite/src/node/plugins/html.ts

+      if (typeof val === 'string') {
+        try {
+          env[key.slice(16)] = JSON.parse(val)
+        } catch {} // ignore non-JSON.parse-able values
+      } else {
+        env[key.slice(16)] = JSON.stringify(val)
+      }


Or maybe we should just support JSON.stringified strings?

Suggested change

if (typeof val === 'string') {

try {

env[key.slice(16)] = JSON.parse(val)

} catch {} // ignore non-JSON.parse-able values

} else {

env[key.slice(16)] = JSON.stringify(val)

}

if (typeof val === 'string') {

try {

env[key.slice(16)] = JSON.parse(val)

} catch {} // ignore non-JSON.parse-able values

}

Yes, I think it is better we only support stringified object/strings. It isn't intuitive from a HTML-only perspective, but IMO it is better to be consistent between JS and HTML here.

I wonder if we really need to support define for HTML though, or we could remove this 958-964 and add a note in the docs if this is confusing for users 🤔
In JS, it makes sense because we replace the whole import.meta.env.VITE_STRING

define: { 'import.meta.env.VITE_STRING': JSON.stringify('string'), },

But in HTML we have %VITE_STRING%. Supporting define in HTML feels like formalizing that define for import.meta.env.VITE_XXX is not a replacement that happens before the regular env replacement, but a way to overwrite env variables in the config. If we would like this feature, maybe we should have a new envOverrides config option?

I use the define for HTML to have a document title from the first page load and reuse the same variable through my js code.

If we don't support it, I would have to hard code the value in HTML and add the same value in a js constant.

Non JSON.stringified strings break the code already here.

So I think it is fair to discard not stringified strings with a warning log maybe?

I think this shows that supporting define in HTML isn't being used as intended though. We wanted to only add support for replacing env variables. And you are going through a define to override a non-existent env variable to be able to use this feature. I think it is better for you to use an inline plugin and transformIndexHtml.

I think we did for JS, it feels a bit of a stretch to me to also support it in HTML. But IIRC, there was a discussion about allowing env to be a config object to have these kind of features (env.declare or env.define maybe)? Maybe

define: { 'import.meta.env.VITE_STRING': JSON.stringify('string'), },

looks intuitive to others. I'm fine if both you and @sapphi-red would like to move on with this PR, as we'll still need to do the breaking change to remove support for this.

I think overloading define to extend import.meta.env.* is fine since it's not a common thing to do, but happy to discuss more about it if it's confusing. The HTML case is indeed a bit tricky but I think a fix like this could be good enough for now.

EDIT: I didn't notice the PR is marked as a breaking change. I'm slightly leaning towards maybe not, depending on how safe we want to play it 😄

I'm leaning towards applying this suggestion (#13425 (comment)).
We document the behavior as "Any properties in import.meta.env can be used in HTML files with a special %ENV_NAME% syntax". I understood it to mean replacing %ENV_NAME% with the value of the element in import.meta.env. So it's the value when executed by JS and not the value written in the source code. From that standpoint, we might say that this is not a breaking change.

I think overloading define to extend import.meta.env.* is fine since it's not a common thing to do, but happy to discuss more about it if it's confusing.

To be honest, a few month ago I didn't know that it works like that. I thought it's a simple replacement but if import.meta.env.* is defined it affects import.meta.env too. (#12866 (comment), #13003)
I think it's confusing that it does more than a simple replacement but I guess it's fine to have this behavior if we add a document about this behavior.

I'm leaning towards applying this suggestion (#13425 (comment)).

What happens for import.meta.env.* that are numbers or booleans though. I think it makes sense to replace e.g. <p>%LEGACY%</p> to <p>false</p>

I was wondering whether it's confusing to allow that. But yeah, it would be better to have access to number/boolean import.meta.envs.

sapphi-red · 2023-06-07T11:13:07Z

I pushed some commits to support values with single/back quotes and to add a warning for ignored ones.

bluwy · 2023-06-09T02:37:03Z

packages/vite/src/node/plugins/html.ts

+      if (typeof val === 'string') {
+        try {
+          env[key.slice(16)] = extractStringLiteral(val)
+        } catch {
+          ignoredImportMetaEnvKeys.add(key.slice(16))
+        }


IIUC extractStringLiteral would prevent usages like:

define: { 'import.meta.env.NULL_STR': 'null' }

(It's a bit of a stretch though 😅)

I wonder if doing:

Suggested change

if (typeof val === 'string') {

try {

env[key.slice(16)] = extractStringLiteral(val)

} catch {

ignoredImportMetaEnvKeys.add(key.slice(16))

}

if (typeof val === 'string') {

try {

env[key.slice(16)] = JSON.parse(val)

} catch {

env[key.slice(16)] = val

}

Would be good enough to prevent the breaking change. I've also made a stackblitz to test things out. Looking back it seems to be a hard feature to balance, and I could be persuaded that the original issue (#13424) could be working as intended too.

Yeah... It's hard.

I removed the warning and made it do the fallback instead. I think it would be intuitive to support single/back quotes so I left the support for them.

I stil don't quite understand why we have special treatment using extractStringLiteral. Couldn't we use JSON.parse here?

define: { 'import.meta.env.DOUBLE_QUOTE': "'foo'", 'import.meta.env.SINGLE_QUOTE': "'foo'", 'import.meta.env.BACK_QUOTE': "`foo`" }

If we use JSON.parse, import.meta.env.SINGLE_QUOTE will be replaced with 'foo'. But I think people will expect that to be replaced with foo like import.meta.env.DOUBLE_QUOTE works.

extractStringLiteral + fallback JSON.parse + fallback

DOUBLE_QUOTE foo foo

SINGLE_QUOTE foo 'foo'

BACK_QUOTE foo `foo`

I think that's fine for now to reduce complexity, they can workaround it by using "\"foo\"" (which is usually what JSON.stringify generates) 🤔 I'm mostly concern of the additional code here that's needed to maintain for a (seemingly) simple feature.

oarsheo · 2023-07-02T11:48:29Z

Code Review AI: packages/vite/src/node/plugins/html.ts

Error handling: The extractStringLiteral function throws errors when it encounters unexpected input. This could potentially halt the execution of the program. It would be better to handle these errors gracefully, perhaps by logging a warning and returning a default value.

function extractStringLiteral(source: string): string {
  try {
    // existing code
  } catch (error) {
    console.warn(`Failed to extract string literal from source: ${source}`);
    return source;
  }
}

Magic numbers: The code contains a magic number 16 in key.slice(16). It would be better to replace this with a named constant to improve readability.

const IMPORT_META_ENV_LENGTH = `import.meta.env.`.length;

// later in the code
env[key.slice(IMPORT_META_ENV_LENGTH)] = JSON.stringify(val);

patak-cat · 2023-07-03T14:09:44Z

@oarsheo please avoid sending AI-generated code reviews to the Vite repo. Thanks!

sapphi-red · 2023-07-24T02:46:31Z

packages/vite/src/node/plugins/html.ts

+          const parsed = JSON.parse(val)
+          env[key.slice(16)] = typeof parsed === 'string' ? parsed : val


I totally forgot about this PR 😅
I replaced extractStringLiteral with this one.
The typeof parsed === 'string' thing is to avoid a breaking change when define has something like 'import.meta.env.VITE_OBJECT_STRING': '{ "foo": "bar" }'.

bluwy

Thanks for making the change! I think we can mark this as a fix() too?

patak-cat · 2023-07-24T08:11:46Z

I'm fine with moving forward with this fix, even if I still have doubts about the way we are extending define to play nicely with import.meta.env. @bluwy would you merge this one if you'd like to see it in a patch?

fix(html): import.meta.env replacement without quotes

1401d63

sapphi-red added p4-important Violate documented behavior or significantly improves performance (priority) breaking change labels Jun 5, 2023

sapphi-red commented Jun 5, 2023

View reviewed changes

patak-cat requested a review from bluwy June 5, 2023 14:36

sapphi-red added 3 commits June 7, 2023 19:26

fix: ignore non-string parsed values

6f0fe03

feat: add warning

dfc4a70

fix: support literal by quote and template literal

0c0eca9

bluwy reviewed Jun 9, 2023

View reviewed changes

feat: fallback

53cf4e5

sapphi-red changed the title ~~fix(html)!: import.meta.env replacement without quotes~~ feat(html): support import.meta.env define replacement without quotes Jun 13, 2023

sapphi-red removed the breaking change label Jun 13, 2023

Tobbe mentioned this pull request Jul 18, 2023

Expose environment variables also on import.meta.env redwoodjs/graphql#8928

Merged

refactor: use JSON.parse instead of extractStringLiteral

233e2bf

sapphi-red commented Jul 24, 2023

View reviewed changes

sapphi-red requested review from bluwy and patak-cat July 24, 2023 02:54

bluwy approved these changes Jul 24, 2023

View reviewed changes

sapphi-red changed the title ~~feat(html): support import.meta.env define replacement without quotes~~ fix(html): support import.meta.env define replacement without quotes Jul 24, 2023

patak-cat approved these changes Jul 24, 2023

View reviewed changes

bluwy merged commit 883089c into vitejs:main Jul 24, 2023

zonemeen mentioned this pull request Jul 25, 2023

chore(vite): support define replacement without removing double quotes vuejs/vitepress#2677

Merged

sapphi-red deleted the fix/html-import-meta-env branch July 25, 2023 09:55

bluwy mentioned this pull request Oct 12, 2023

feat(define): handle replacement with esbuild #11151

Merged

9 tasks

	`extractStringLiteral` + fallback	`JSON.parse` + fallback
DOUBLE_QUOTE	`foo`	`foo`
SINGLE_QUOTE	`foo`	`'foo'`
BACK_QUOTE	`foo`	`foo`

		const parsed = JSON.parse(val)
		env[key.slice(16)] = typeof parsed === 'string' ? parsed : val

Uh oh!

Conversation

sapphi-red commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

bolt-new-by-stackblitz bot commented Jun 5, 2023

Uh oh!

sapphi-red Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patak-cat Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yannbriancon Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluwy Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapphi-red Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapphi-red commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oarsheo commented Jul 2, 2023

Uh oh!

patak-cat commented Jul 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sapphi-red commented Jun 5, 2023 •

edited

Loading

sapphi-red Jun 5, 2023 •

edited

Loading

patak-cat Jun 5, 2023 •

edited

Loading

yannbriancon Jun 5, 2023 •

edited

Loading

bluwy Jun 5, 2023 •

edited

Loading

sapphi-red Jun 7, 2023 •

edited

Loading