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(compiler-sfc): analyze export default by AST #7068

Merged
merged 3 commits into from
Mar 28, 2023
Merged

fix(compiler-sfc): analyze export default by AST #7068

merged 3 commits into from
Mar 28, 2023

Conversation

sxzz
Copy link
Member

@sxzz sxzz commented Nov 9, 2022

closes #7038, closes #7041, closes #7078

vite plugin PR vitejs/vite#10888


thanks to @btea

packages/compiler-sfc/src/rewriteDefault.ts Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Nov 9, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit c6f252a
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/63d2b55c6cc5670009a75db8

Copy link
Member

@znck znck left a comment

Choose a reason for hiding this comment

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

I think it's safer if we use as __VUE_DEFAULT__ in the else case (see highlighted line in the screenshot below).

image

@sxzz
Copy link
Member Author

sxzz commented Nov 10, 2022

@znck Done.

@znck znck added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged. labels Nov 11, 2022
@sxzz sxzz added ready to merge The PR is ready to be merged. and removed ready to merge The PR is ready to be merged. labels Nov 11, 2022
Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

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

A problem with this approach: rewriteDefault is used in @vitejs/plugin-vue here, on the script string already transformed by compileScript. This means the string no longer matches the Node positions in the AST exposed on the SFC descriptor.

I think we need to do something similar to reactivity transform's transformAST:

  1. Accept both the AST and the corresponding MagicString instance in rewriteDefault
  2. Also expose the MagicString instance on the script block returned by compileScript
  3. In @vitejs/plugin-vue, feature detect by checking whether MagicString instance is available
  • If yes, use AST-based rewrite by passing in AST + MagicString
  • If no, fallback to old usage

@yyx990803
Copy link
Member

@sxzz did you end up re-creating vitejs/vite#10888 for https://github.com/vitejs/vite-plugin-vue?

@sxzz
Copy link
Member Author

sxzz commented Mar 28, 2023

@yyx990803 No, I haven't done it yet.

@sxzz
Copy link
Member Author

sxzz commented Mar 28, 2023

@yyx990803 yyx990803 merged commit 701b95f into vuejs:main Mar 28, 2023
@sxzz sxzz deleted the fix/rewrite-default-ast branch March 28, 2023 03:59
IAmSSH pushed a commit to IAmSSH/core that referenced this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage.
Projects
None yet
3 participants