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.

Sign up for GitHub

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

Jump to bottom

fix(html): support import.meta.env define replacement without quotes #13425

Merged
merged 6 commits into from
Jul 24, 2023
Merged

fix(html): support import.meta.env define replacement without quotes #13425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1401d63
fix(html): import.meta.env replacement without quotes
sapphi-red Jun 5, 2023
6f0fe03
fix: ignore non-string parsed values
sapphi-red Jun 7, 2023
dfc4a70
feat: add warning
sapphi-red Jun 7, 2023
0c0eca9
fix: support literal by quote and template literal
sapphi-red Jun 7, 2023
53cf4e5
feat: fallback
sapphi-red Jun 13, 2023
233e2bf
refactor: use JSON.parse instead of extractStringLiteral
sapphi-red Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,13 @@ export function htmlEnvHook(config: ResolvedConfig): IndexHtmlTransformHook {
for (const key in config.define) {
if (key.startsWith(`import.meta.env.`)) {
const val = config.define[key]
env[key.slice(16)] = typeof val === 'string' ? val : JSON.stringify(val)
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)
}
Copy link
Member Author

@sapphi-red sapphi-red Jun 5, 2023 •
edited

Choose a reason for hiding this comment

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

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
}

Copy link
Member

@patak-dev patak-dev Jun 5, 2023 •
edited

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bluwy bluwy Jun 5, 2023 •
edited

Choose a reason for hiding this comment

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

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member Author

@sapphi-red sapphi-red Jun 7, 2023 •
edited

Choose a reason for hiding this comment

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

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.

}
}
return (html, ctx) => {
Expand Down
1 change: 1 addition & 0 deletions playground/html/__tests__/html.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ describe('env', () => {
test('env works', async () => {
expect(await page.textContent('.env')).toBe('bar')
expect(await page.textContent('.env-define')).toBe('5173')
expect(await page.textContent('.env-define-string')).toBe('string')
expect(await page.textContent('.env-bar')).toBeTruthy()
expect(await page.textContent('.env-prod')).toBe(isBuild + '')
expect(await page.textContent('.env-dev')).toBe(isServe + '')
Expand Down
1 change: 1 addition & 0 deletions playground/html/env.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<p class="env">%VITE_FOO%</p>
<p class="env-define">%VITE_NUMBER%</p>
<p class="env-define-string">%VITE_STRING%</p>
<p class="env-%VITE_FOO%">class name should be env-bar</p>
<p class="env-prod">%PROD%</p>
<p class="env-dev">%DEV%</p>
Expand Down
1 change: 1 addition & 0 deletions playground/html/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export default defineConfig({

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

plugins: [
Expand Down