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

Differentiate script setup from option api component with setup #1928

Conversation

renatodeleao
Copy link
Contributor

@renatodeleao renatodeleao commented Jan 3, 2023

After a lot of discussion at #1869 and failed attempts to reproduce the issue I finally made the error happen and propose a fix. It's very ugly, but the tests are green and make more VTU@2 "customers" happy (at least one customer :) )

The patch

Using yet another vue internal property __isScriptSetup12 to differentiate <script setup> components from Option API components with setup() hook since both fall in the hasSetupState block. The latter can be patched regularly without triggering Proxy trap set error that lead to the 2.2.3 patch in the first place (#1861), and only when patched like that, it actually works in all possible scenarios.

The only reason for this to be acceptable code is because the previous implementation also relied on devtoolsRawSetupState which is also considered internal property.

Before

test-utils/src/mount.ts

Lines 481 to 485 in a48712d

// we need to differentiate components that are or not not `script setup`
// otherwise we run into a proxy set error
// due to https://github.com/vuejs/core/commit/f73925d76a76ee259749b8b48cb68895f539a00f#diff-ea4d1ddabb7e22e17e80ada458eef70679af4005df2a1a6b73418fec897603ceR404
// introduced in Vue v3.2.45
if (hasSetupState(this)) {

After

test-utils/src/mount.ts

Lines 485 to 488 in aec9623

// Also ensures not to include option API components in this this block
// since they can also have setup state but need to be patched using
// the regular method.
if (hasSetupState(this) && this.$.setupState.__isScriptSetup) {

Notes for the reviewer

  • I only tested the code with the current vue@3.2.45 of the codebase.
  • If we to keep the git history clean, feel free to rebase and squash both commits, I've kept them to provide an easy way to reset and QA the PR. git reset HEAD^ --hard && npm run test mocks to see the spec failing.
  • If the solution is accepted, it's probably better to just rename the helper hasSetupState to isScriptSetup and move the && check within the helper. We can also check if simply using the __isScriptSetup flag is enough.

Footnotes

  1. https://github.com/vuejs/core/blob/0c07f12541061cf10996f998705d7ad36193aa4f/packages/runtime-core/__tests__/apiCreateApp.spec.ts#L517

  2. https://github.com/vuejs/core/blob/8a123ac34fd30fc36ac9e0c252dd345d6d7c7f50/packages/compiler-core/src/options.ts#L121

@netlify
Copy link

netlify bot commented Jan 3, 2023

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit f509cef
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/63b551917b442a00083eb9e8
😎 Deploy Preview https://deploy-preview-1928--vue-test-utils-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@renatodeleao renatodeleao changed the title Patch/#1869 differentiate script setup from option api component with setup Differentiate script setup from option api component with setup Jan 3, 2023
the test from b7be04a should now pass, as well as any other test from the suite.
@renatodeleao renatodeleao force-pushed the patch/#1869-differentiate-script-setup-from-option-api-component-with-setup branch from 7b45b79 to aec9623 Compare January 3, 2023 19:02
Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @renatodeleao 👍

Your fix looks good to me, I was afraid we would need more cases to make everything work.
As I commented in the review, let's try just using _isScriptSetup and see if that's OK.

src/mount.ts Outdated
// Also ensures not to include option API components in this this block
// since they can also have setup state but need to be patched using
// the regular method.
if (hasSetupState(this) && this.$.setupState.__isScriptSetup) {
Copy link
Member

Choose a reason for hiding this comment

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

I like your suggestion to have a isScriptSetup util function 👍
And maybe this check can just rely on _isScriptSetup and we can remove hasSetupState here.
I think hasSetupState is used elsewhere and is really needed to know if the component has a setup (but not necessarily a script setup). If tests are green, we can cut a release and give it a go.

Copy link
Member

Choose a reason for hiding this comment

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

nit: typo this this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you're right! I've removed the hasSetupState check from here because it's redundant, but we can't replace hasSetupState with isScriptSetup as they check for different things and some specs around expose fail if we did.

Thanks for the review!

src/mount.ts Outdated Show resolved Hide resolved
…se it

- drop hasSetupState check, if a component is <script setup> i always has setup state
- do note that isScriptSetup and hasSetupState check for different things. I tried to replaced hasSetupScript with isScriptSetup but some specs around expose started to fail so we are required to keep it for now.
@renatodeleao renatodeleao force-pushed the patch/#1869-differentiate-script-setup-from-option-api-component-with-setup branch from 39d2312 to f509cef Compare January 4, 2023 10:14
@cexbrayat cexbrayat merged commit a3a3e80 into vuejs:main Jan 4, 2023
@cexbrayat
Copy link
Member

Thanks @renatodeleao , we'll cut a release soon

@renatodeleao renatodeleao deleted the patch/#1869-differentiate-script-setup-from-option-api-component-with-setup branch January 5, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants