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-core): No access this through function declaration in <script setup> #6827

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

godxiaoji
Copy link
Contributor

fix #6822

Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

Could you please add unit tests of the issue?

@godxiaoji
Copy link
Contributor Author

Could you please add unit tests of the issue?

Case added. Thanks.

if (
parent &&
parent.type === 'CallExpression' &&
(type === BindingTypes.SETUP_LET || type === BindingTypes.SETUP_CONST)
Copy link
Member

@sxzz sxzz Oct 6, 2022

Choose a reason for hiding this comment

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

Why check BindingTypes here? Consider this example, the problem is still existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I had to make sure it's a function identifier with parents.type. Then use .bind() to change the scope to undefined.
  2. Considered with the smallest dimension of the issue, including the following situations when type is BindingTypes.SETUP_CONST or BindingTypes.SETUP_LET:
// BindingTypes.SETUP_LET
let foo = () => {}
let foo = function() {}

// BindingTypes.SETUP_CONST
const foo = () => {}
const foo = function {}
function foo() {}

Copy link
Member

Choose a reason for hiding this comment

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

There are many kinds of BindingTypes. For generality, I don't think it's necessary to check BindingTypes here.

Or if you have a better solution to fix the example I provided, it's also all right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! The previous judgment was poor, and the following conditions are also met.

const foo = ref(function(a, b) {
  console.log(this)
})

Copy link
Member

Choose a reason for hiding this comment

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

Great, but CI is failed. You can push an empty commit git commit --allow-empty -m "Empty-Commit" to re-run CI.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks! It's actually the other way around, it should fail in dev (like it does in prod): vitejs/vite#3872 (comment)

@sxzz
Copy link
Member

sxzz commented Oct 6, 2022

@posva This PR is true to fix - it will fail in both dev and prod mode.

@godxiaoji godxiaoji requested a review from posva October 6, 2022 13:53
@posva
Copy link
Member

posva commented Oct 6, 2022

You are right, I was a bit too fast on reading that .bind()

Copy link

pkg-pr-new bot commented Oct 22, 2024

Open in Stackblitz

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@6827

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@6827

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@6827

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@6827

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@6827

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@6827

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@6827

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@6827

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@6827

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@6827

vue

pnpm add https://pkg.pr.new/vue@6827

commit: 6ebd836

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB 38 kB 34.2 kB
vue.global.prod.js 159 kB 57.9 kB 51.4 kB

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.9 kB 18.3 kB 16.7 kB
createApp 55 kB 21.3 kB 19.4 kB
createSSRApp 59 kB 23 kB 20.9 kB
defineCustomElement 59.8 kB 22.8 kB 20.8 kB
overall 68.7 kB 26.3 kB 24 kB

@edison1105 edison1105 added 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. scope: script-setup ready for review This PR requires more reviews labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready for review This PR requires more reviews scope: script-setup
Projects
Development

Successfully merging this pull request may close these issues.

component is accessible by this from function declared in <script setup> on dev mode
4 participants