fix(build): handle invalid JSON in import.meta.env#17648
Conversation
|
|
|
@bluwy Could you please take some time to review this pull request? |
|
@bluwy I saw that you have viewed this PR. Is it worth merging? Are there any failures? |
|
I have a few concerns with this trick.
Maybe we need a different approach to solve this. Perhaps we can simply replace And we can build a regex with
When the user does |
|
Ok, got it. I'll try to implement this. |
bb9912c to
b94ce8d
Compare
b94ce8d to
d7eb09b
Compare
|
@bluwy I have finished this. Now it will replace the undefined |
|
@bluwy Now it replaces I think that there is no need to capture the key. If there still has any keys, the keys' supposed to be undefined in |
Good point, that's a nice optimization.
I don't think we should do this because sourcemap handling is very expensive. I'll push some commits and test locally if we can apply the |
It should replace the undefined key with undefined after |
|
I pushed the commits to optimize the sourcemap handling. I agree that bare I also verified that the |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI on
✅ analogjs, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress |
|
I saw that there are some ecosystem-ci fail. Is there anything I should do? |
|
You can ignore them. The "latest scheduled" are the results from main, so this PR isn't causing those fails. I'm waiting for another review before merging this. |
patak-cat
left a comment
There was a problem hiding this comment.
Great fix! I think we could merge it in 5.4 just to play safe.
|
Okay. Thanks to everyone for reviewing this pr! |
|
It seems that since this change, I'm running into What's also unexpected is that this |
Please provide a issue to describe the details. |
|
I have a hard time creating a reproduction for this (and issues without reproduction would be rejected), but here is the full error: The compiled output of the lib (which has Edit: WIP reproduction at https://github.com/silverwind/vite-import-meta, but does not reproduce yet. |
|
Is the library has error named |
|
I think one scenario this can happen is when library code (or its dependencies) include both // lib/index.js
export const foo = () => {
// https://github.com/pmndrs/zustand/blob/a56e76db5261291dcf9a88573dac58f67edb93db/rollup.config.js#L67-L70
console.log(import.meta.env ? import.meta.env.MODE : undefined);
console.log(process.env.NODE_ENV);
};
// ⇓ vite library build
// lib/dist/index.js
const __vite_import_meta_env__ = { "BASE_URL": "/", "DEV": false, "MODE": "production", "PROD": true, "SSR": false };
const foo = () => {
console.log(__vite_import_meta_env__ ? "production" : void 0);
console.log(process.env.NODE_ENV);
};
export {
foo
};Then later in main app build, |
|
But is there any need to use bare There is no need to use conditional operation in your code. If you use vite to bundle it, it will replace |
To clarify, this is not "my code" but just some examples in the wild ( Another package I remembered which might causes an issue is unjs's std-env https://github.com/unjs/std-env/blob/b4ef16832baf4594ece7796a2c1805712fde70a3/src/env.ts. It uses a bare |
|
Maybe we should create an issue to import { transform } from 'esbuild'
const result = await transform(
`console.log(import.meta.env?.MODE)`,
{
sourcemap: false,
minify: true,
target: ['chrome60'],
define: {
'import.meta.env.MODE': JSON.stringify('production'),
},
}
)
console.log(result.code)It will ouput |
|
Yes, it is zustand as well in my case as well which uses
I think it's fine for zustand to do this. They are logging some runtime deprecation warnings, but only during dev mode. |
|
Okay, I'll try to reproduce it and make a new pr. |
|
I've created #17874 to track this, let's move the conversation over there |
chore: repro for vitejs/vite#17648
Bug Reproduction
fix #17710
NOTE(bluwy): the solution description below is outdated. The new solution is based on #17648 (comment)
When
import.meta.envhas some invalid JSON value, such as__VITE_IS_LEGACY__imported by@vitejs/plugin-legacy, Vite will replace theimport.meta.envinimport.meta.env.SOMEVAR_UNDEFINEDto a plain object. Then the javascript file will be an invalid javascript file which will produce a error when handled by proceedingcommonjsplugin.The minimum reproducible set is here: https://stackblitz.com/edit/stackblitz-starters-xr8lwk?file=index.html
Reason
As previous description,
vite:defineplugin will replaceimport.meta.env.SOMEVAR_UNDEFINEDto{"BASE_URL": "/", "MODE": "development", "DEV": true, "PROD": false, "SSR": false, "LEGACY": __VITE_IS_LEGACY__}.SOMEVAR_UNDEFINED.The reason is,
vite:definewill replace the actualimport.meta.envwith a marker.vite/packages/vite/src/node/plugins/define.ts
Lines 144 to 148 in 167006e
The original value of
import.meta.envis a object, while the marker is an identifier. The replace strategy ofesbuildto handle this two type is different.Consider the following example.
When
import.meta.envis a identifier,esbuildwill replace it in place.After that,
vite:definewill replace the marker with the originalimport.meta.envvite/packages/vite/src/node/plugins/define.ts
Lines 181 to 183 in 167006e
Now it's a invalid javascript code.
Solution
Align the type of the original and marker.
I generate a marker which is a valid JSON, so that the replace strategy will be same.
Radical Solution
I found that the value of
import.meta.envis spread in config.{ "loader": "js", "charset": "utf8", "platform": "neutral", "define": { "process.env": "{}", "global.process.env": "{}", "globalThis.process.env": "{}", "process.env.NODE_ENV": "\"production\"", "global.process.env.NODE_ENV": "\"production\"", "globalThis.process.env.NODE_ENV": "\"production\"", "import.meta.hot": "undefined", "import.meta.env.BASE_URL": "\"/\"", "import.meta.env.MODE": "\"production\"", "import.meta.env.DEV": "false", "import.meta.env.PROD": "true", "import.meta.env.SSR": "false", "import.meta.env.LEGACY": "__VITE_IS_LEGACY__", "import.meta.env": "_ecc578fd856ad0ab7198de04a00b9405c7a6d2549217f51adfff717ee9f7b851______________________________________________" }, "sourcefile": "/Users/yuzheng14/code/repositories/legacy-conflict/node_modules/.pnpm/vue@2.7.16/node_modules/vue/dist/vue.runtime.esm.js", "sourcemap": false }And
esbuildwill replace it first.Under this case,
import.env.metawill be only be replace when the value of it is not defined. So why not leave a{}forimport.meta.envjust likeprocess.env?vite/packages/vite/src/node/plugins/define.ts
Lines 20 to 27 in 167006e
But I couldn't aware the side effects the this solution, so that I choose to modify the generation of marker.