-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
fix(ssr): Transform named default exports without altering scope #4053
Conversation
// anonymous default exports | ||
s.overwrite( | ||
node.start, | ||
node.start + `export default`.length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The + 14
in the original code seemed like a horrible magic number, which is why I replaced it with a string length operation instead. If this is a cause of concern for performance, we can convert it back, I guess?
expect((await ssrTransform(`export default class {}\n`, null, null)).code) | ||
.toMatchInlineSnapshot(` | ||
"__vite_ssr_exports__.default = class {} | ||
" | ||
`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why, but stripping out the newline from the source + target caused toMatchInlineSnapshot
to insist that the lines were not equivalent. There might be a bug with its handling of single-line code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could apply .trim()
to code
before toMatchInlineSnapshot
Post NotesPrior to landing on __vite_ssr_exports__.default = function() {} __vite_ssr_exports__.default = class {} function hello() {}
hello.test = 1234;
__vite_ssr_exports__.default = hello; class A {}
class B extends A {}
__vite_ssr_exports__.default = hello;
Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }}) Unfortunately, this actually failed some tests with Vue SSR, probably because it doesn't quite replicate the proper export defaults. As such, I wonder if it may be worth considering Object.defineProperty(__vite_ssr_exports__, "default", { enumerable: true, value: __vite_ssr_exports__.default }) That said, no tests are failing without it, and I can't think of one that would fail without it right now. If someone has any concerns regarding this, please do comment on the PR. |
…ejs#4053) Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Fixes #4049.
Existing Behavior
Previously, given the following source files:
The SSR Transformer would indiscriminately replace the
export default
with__vite_ssr_exports__.default =
as follows:Which works for the anonymous cases, but alters the scope of named functions and classes, producing erroneous code that references undefined bindings.
Proposed Behavior
This PR instead reuses the
Object.defineProperty
technique already used for named exports:The options passed to
Object.defineProperty
are the same ones performed by TypeScript'sesModuleInterop
. The anonymous case is still handled in the same manner as the previous algorithm.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).