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.

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

Merged
merged 2 commits into from Jul 2, 2021

Conversation

GrygrFlzr
Copy link
Member

Fixes #4049.

Existing Behavior

Previously, given the following source files:

export default function() {}
export default class {}
export default function hello() {}
hello.test = 1234;
export default class A {}
export class B extends A {}

The SSR Transformer would indiscriminately replace the export default with __vite_ssr_exports__.default = as follows:

__vite_ssr_exports__.default = function() {}
__vite_ssr_exports__.default = class {}
__vite_ssr_exports__.default = function hello() {}
hello.test = 1234;
__vite_ssr_exports__.default = class A {}
class B extends A {}
Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }})

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:

__vite_ssr_exports__.default = function() {}
__vite_ssr_exports__.default = class {}
function hello() {}
hello.test = 1234;
Object.defineProperty(__vite_ssr_exports__, "default", { enumerable: true, value: hello })
class A {}
class B extends A {}
Object.defineProperty(__vite_ssr_exports__, "default", { enumerable: true, value: A })
Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }})

The options passed to Object.defineProperty are the same ones performed by TypeScript's esModuleInterop. The anonymous case is still handled in the same manner as the previous algorithm.


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 Commit 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.

// anonymous default exports
s.overwrite(
node.start,
node.start + `export default`.length,
Copy link
Member Author

@GrygrFlzr GrygrFlzr Jun 30, 2021

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?

Comment on lines +231 to +235
expect((await ssrTransform(`export default class {}\n`, null, null)).code)
.toMatchInlineSnapshot(`
"__vite_ssr_exports__.default = class {}
"
`)
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 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?

Copy link
Member

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

@GrygrFlzr
Copy link
Member Author

GrygrFlzr commented Jun 30, 2021

Post Notes

Prior to landing on Object.defineProperty, I did try to just append __vite_ssr_exports__.default = name at the end like so:

__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 even for the anonymous cases like so:

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.

patak-dev
patak-dev previously approved these changes Jun 30, 2021
@patak-dev patak-dev requested a review from antfu June 30, 2021 13:41
antfu
antfu previously approved these changes Jul 2, 2021
@antfu antfu dismissed stale reviews from patak-dev and themself via 5f72011 July 2, 2021 09:30
@antfu antfu merged commit 5211aaf into main Jul 2, 2021
@antfu antfu deleted the named-default-exports branch July 2, 2021 09:35
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR: References to named default export functions get out of scope
3 participants