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(types/ref): allow getter and setter types to be unrelated #11442

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

jh-leong
Copy link
Member

close #6766

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90 kB 34.5 kB 31.1 kB
vue.global.prod.js 147 kB 54 kB 48.1 kB

Usages

Name Size Gzip Brotli
createApp 49.8 kB 19.5 kB 17.8 kB
createSSRApp 53.3 kB 21 kB 19.1 kB
defineCustomElement 52 kB 20.3 kB 18.5 kB
overall 63.3 kB 24.5 kB 22.3 kB

@jh-leong

This comment was marked as resolved.

@jh-leong jh-leong marked this pull request as draft July 27, 2024 05:13
@jh-leong jh-leong marked this pull request as ready for review July 28, 2024 08:35
@jh-leong
Copy link
Member Author

Hello team, could you please help run the ecosystem-ci? Thanks!

@yyx990803
Copy link
Member

/ecosystem-ci run

@yyx990803
Copy link
Member

@jh-leong I've invited you to the team btw. Thank you for your contributions!

@vue-bot
Copy link
Contributor

vue-bot commented Jul 29, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt success success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify success failure
vueuse success success
vue-simple-compiler success success

@yyx990803 yyx990803 merged commit e0b2975 into vuejs:main Jul 29, 2024
11 checks passed
@jh-leong jh-leong deleted the fix/ref branch July 29, 2024 02:54
@haoqunjiang
Copy link
Member

@johnsoncodehk
This PR has affected the generated type signature of https://github.com/vuejs/language-tools/blob/dc6f943c6ccb26778906a49532966aaf69fdc5a5/test-workspace/component-meta/reference-type-exposed/component.vue#L10 (full log at https://github.com/vuejs/ecosystem-ci/actions/runs/10201662987/job/28224044101#step:7:1006)
Could you update the vue version and the test snapshot in the language-tools repo so that we can make the ecosystem-ci all green again? Thanks!

cativovo added a commit to cativovo/kalendaryo that referenced this pull request Aug 4, 2024
@mefcorvi
Copy link
Contributor

mefcorvi commented Aug 5, 2024

@jh-leong @yyx990803
It appears this PR has changed the type inference for ref when used with MaybeRef:

import { ref, MaybeRef } from 'vue'

const a: MaybeRef<number> = 1;
const b = ref(a); // Ref<number, MaybeRef<number>>

Shouldn't it be Ref<number, number> instead?

Consider this example as well:

const a: MaybeRef<number> = ref(1);
const b = ref(a); // Ref<number, number | Ref<number, number>>

@jh-leong
Copy link
Member Author

jh-leong commented Aug 6, 2024

Hi @mefcorvi, thanks for your feedback.

The behavior in the examples you mentioned is expected, see TypeScript Playground.

I'm not entirely sure what specific issue you're referring to. Could you please create a new issue and provide a concrete example that illustrates the problem? This would help us track and address the issue more effectively.

Thanks.

@mefcorvi
Copy link
Contributor

mefcorvi commented Aug 6, 2024

To be honest, I didn't expect that it was possible to put one ref inside another ref. I was sure that in such cases there would be unwrapping, i.e., b2.value = ref(1) would be equivalent to b2.value = 1, since ref(ref(1)).value === 1. Now I see that this is a valid case, and it is actually possible to have one ref inside another, so the current typing is indeed correct.

I apologize for the confusion and any inconvenience this may have caused. My question is resolved.

@mefcorvi
Copy link
Contributor

mefcorvi commented Aug 6, 2024

@jh-leong After some thought, I conclude that something is indeed broken. Now, it is possible to have code like this, which is correct from a types standpoint but obviously broken. Consider this example:

const b = ref(ref(1));
console.log(b.value.toFixed()); // 1
b.value = ref(2);
console.log(b.value.toFixed()); // runtime error

Here, the type of b is Ref<number, number | Ref<number>>. This means that we can set it to Ref<number>, but the getter is supposed to return number. However, if we set b.value to ref(1), we actually get ref(1) back.

Additionally, I believe that before the change in the current PR, it was not possible to set the value of a ref to another ref because of both setters and getters were UnwrapRef. Looking at the implementation of the ref function, I don't think that having one ref inside another was ever an allowed case.

I will create a separate issue since this PR is already merged.

@sxzz
Copy link
Member

sxzz commented Aug 7, 2024

#8543 (review)

I noticed this PR was merged into main instead of minor. This may require TS > 5.1 in the next patch, so we should move it to minor.

yyx990803 added a commit that referenced this pull request Aug 7, 2024
…lated (#11442)"

This reverts commit e0b2975.

This change requires TypeScript 5.1 so it is moved to a minor release.
@yyx990803
Copy link
Member

@jh-leong I've reverted this and #11536 in the main branch so we avoid accidentally raising TS version requirements in 3.4.

minor branch has merged main with both this and #11442 applied so they will be part of 3.5.

@Cr0zy07
Copy link

Cr0zy07 commented Aug 8, 2024

@jh-leong I've reverted this and #11536 in the main branch so we avoid accidentally raising TS version requirements in 3.4.

minor branch has merged main with both this and #11442 applied so they will be part of 3.5.

I think the main branch has been merged into minor after the revert. I could be wrong, but I don't see both of them in the beta release.

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.

Using generic for ref() causes error when reassigning
7 participants