-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
test(reactivity): support passing object with only one get property #9122
base: main
Are you sure you want to change the base?
Conversation
Size ReportBundles
Usages
|
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.
Why? It's necessary.
If it's necessary and tests still pass after removing it maybe there should be a test for this? |
Yes. Add the case: import { computed, isReadonly } from 'vue'
const a = computed(() => 1)
// @ts-expect-error
const b = computed({ get: () => 1 })
console.log(isReadonly(a), isReadonly(b))
// Both should be true |
If the ts type is strictly adhered to, then this setter is unnecessary. If it is not strictly adhered to, should the key of the |
We encourage users to use I think keeping existing cases is for greater robustness. |
1aba386
to
b218426
Compare
@@ -151,6 +151,14 @@ describe('reactivity/computed', () => { | |||
expect(n.value).toBe(-1) | |||
}) | |||
|
|||
it('should support passing object with only one get property', () => { | |||
const a = computed(() => 1) |
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 think the scenario tested by a
is already tested by it('should be readonly'
. I think that part can be removed here.
I also wonder whether the test for b
should just be merged into it('should be readonly'
? Or, if not, maybe move this test case to be next to that one?
No description provided.