Exporting redefined "Object" throws an error in SSR

sheremet-va commented 10 months ago

Describe the bug

When Object variable is redefined and exported from a module, Vite SSR fails with an error:

Error: Cannot read properties of null (reading 'defineProperty')
    at eval (/home/projects/node-m1jzee/test.js:6:8)
    at _0x2e26db._evaluate (https://nodem1jzee-ui1s.w-corp.staticblitz.com/blitz.a2aabdd9.js:352:376700)
    at async ModuleJob.run (https://nodem1jzee-ui1s.w-corp.staticblitz.com/blitz.a2aabdd9.js:181:2372)

Reproduction

https://stackblitz.com/edit/node-m1jzee?file=test.js,index.mjs

Steps to reproduce

node index.mjs

System Info

Stackblitz

Used Package Manager

npm

Logs

No response

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

stackblitz[bot] commented 10 months ago

Start a new pull request in StackBlitz Codeflow.

node-m1jzee (Open reproduction)

XiSenao commented 10 months ago

Does this mean that the injected code in the vite internal needs to import helper to prevent similar program errors?

// Injected code
import { _Object } from 'internal/variables';

const Object = null;
console.log(Object);

// Injected code
_Object.defineProperty(__vite_ssr_exports__, "Object", { enumerable: true, configurable: true, get(){ return _Object }});

sheremet-va commented 10 months ago

This was also my idea, yes. But in this case we can abstract it even more:

import { _export } from '/@vite/ssr'
const Object = null;
console.log(Object);
_export(__vite_ssr_exports__, 'Object', () => Object)

Another solution would be to provide these helpers the same way other variables are injected:

(async function(__vite_ssr_exports__, __vite_ssr_export__) {
const Object = null;
console.log(Object);
__vite_ssr_export__('Object', () => Object)
})()

The problem with the last one is it breaks backward compatibility.

XiSenao commented 10 months ago

Yeah, but I am still a bit hesitant. The helper module may continue to add additional new features in the future. Even if the application may not need all of the helpers, the browser will still load and parse them all. Adding an extra layer of network overhead to help parse the code injected by Vite, I don't know if it's worth it.

sheremet-va commented 10 months ago

SSR code doesn't run in the browser.

XiSenao commented 10 months ago

Sorry, I overlooked the existing issue and approached the problem from a general perspective.

I looked at the other modules and found that there seems to be a similar issue with this logic as well. https://github.com/vitejs/vite/blob/d2aa0969ee316000d3b957d7e879f001e85e369e/packages/vite/src/node/plugins/importAnalysisBuild.ts#L536

bluwy commented 10 months ago

I think making __vite_ssr_export__ as a function is great, but it seems that it's not done that way because of fast-paths that convert export default directly to __vite_ssr_export__.default:

https://github.com/vitejs/vite/blob/b44c49302ffbf0c82f984f6219ed6376d1e4552a/packages/vite/src/node/ssr/ssrTransform.ts#L65-L75

There's also similar issues with URL today too that's hard to fix: https://github.com/vitejs/vite/issues/11853

As a quick fix, maybe we can refer as globalThis.Object instead?

sheremet-va commented 10 months ago

I think making vite_ssr_export as a function is great, but it seems that it's not done that way because of fast-paths that convert export default directly to vite_ssr_export.default

Can you elaborate why this is a problem for this approach? defineProperty is usually not used for default anyway because it's an expression and not a live binding

bluwy commented 10 months ago

I don't think it's a problem, just that the linked code makes the function form a bit cumbersome to use. But in hindsight, maybe it isn't as bad. We need to replace the export default with __vite_ssr_export__('default', and append a ) to close it.

sheremet-va commented 10 months ago

I don't think it's a problem, just that the linked code makes the function form a bit cumbersome to use. But in hindsight, maybe it isn't as bad. We need to replace the export default with __vite_ssr_export__('default', and append a ) to close it.

Ah, I see. I meant adding a new argument, not changing ssr_export type, so default export is unchanged

bluwy commented 10 months ago

Oh, I missed the s difference 😅 I though you mean converting __vite_ssr_exports__ from an object to a function. I think I'd prefer having one way to set the exports instead so it's easier to follow and prevent potentially diverging things.

sheremet-va commented 10 months ago

Oh, I missed the s difference 😅 I though you mean converting __vite_ssr_exports__ from an object to a function. I think I'd prefer having one way to set the exports instead so it's easier to follow and prevent potentially diverging things.

This was just an example of a utility name. From my point of view, there is a module object and there is a separate utility to populate it like exportsAll. Blurring them together would make it more confusing.

vitejs / vite