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

[@vue/reactivity][typing] Unwrapping breaks classes with private fields. #2981

Closed
nandin-borjigin opened this issue Jan 10, 2021 · 4 comments
Labels
has workaround A workaround has been found to avoid the problem need documentation Not necessarily a bug, but proper documentation is needed scope: reactivity scope: types wontfix This will not be worked on

Comments

@nandin-borjigin
Copy link

nandin-borjigin commented Jan 10, 2021

Version

3.0.5

Reproduction link

https://codesandbox.io/s/refvalue-strips-off-all-private-fields-2ws44?file=/src/index.ts

Steps to reproduce

import { ref, Ref } from "@vue/reactivity";

class Foo {
  public bar = 1;
  private baz = "a";
}

// Case 1:
interface FooSerivce {
  readonly foo: Ref<Foo>;
}

class FooServiceImpl implements FooSerivce {
  // error: Type 'Ref<{ bar: number; }>' is not assignable to type 'Ref<Foo>'
  foo = ref(new Foo());
}

// IRRELEVANT: consuming the class to get rid of no-unused-vars warning.
new FooServiceImpl();

// Case 2:
class FooService2 {
  foo = ref(new Foo());
}

const fooService = new FooService2();

function fooUi(props: { foo: Foo }) {
  // mimicing a render mechanism here,
  // it's not relevant to the issue.
  return `<h1>${props.foo.bar}</h1>`;
}

// Type '{ bar: number; }' is not assignable to type 'Foo'
fooUi({ foo: fooService.foo.value });

/**
 * It's true that we won't need those private fields
 * when consuming the returned ref. But the real case of
 * the `Foo` class in this demo is often some class provided
 * by a 3rd party library and we cannot modify it.
 * Further, the private field isn't necessarily direct member of
 * the wrapped variable type, it could be deep.
 * See realistic-example.ts for more realistic example.
 */

What is expected?

Type system is happy with either of the cases.

What is actually happening?

Type system is complaining


A naive suggestion is to change the definition of Ref generic type to truly reflect the return type of ref function, as the problem is caused by the mis-alignment between the actual return type of ref function and the Ref type definition. ref function's return type involves Mapped types while Ref<T> simply has a member value of type T.

@nandin-borjigin nandin-borjigin changed the title [@vue/reactivity][typing] ref(value) strips off private memebers [@vue/reactivity][typing] Ref<T> is not aligned with the return type of ref function Jan 10, 2021
@LinusBorg
Copy link
Member

LinusBorg commented Jan 10, 2021

This is a pretty bad caveat stemming from a combination of things:

This is a limitation in Typescript, when using private properties in classes the Unwrapping that reactivity is doing:

  • For TS, the private baz property is part of the Foo type,even though it's private.
  • When Foo is being unwrapped, the resulting interface does not include the private property because TS does not include private properties when reflecting Foo's properties, and hence, the compiler complains that the types are incompatible.

The code does work at runtime though, as the private field is technically still "there", but for TS to be happy, you need to help TS out:

A. Unwrap it:

interface FooService {
  readonly foo: Ref<UnwrapRef<Foo>>;
}

B. Typecast it:

class FooServiceImpl implements FooService {
  foo = ref(new Foo()) as Ref<Foo>;
}

In both instances though, you would need to cast it back to its original value when passing it to something that expects an instance of Foo:

fooUi({ foo: fooService.foo.value as Foo });

None of this is ideal, but short of dropping the Unwrapping behavior, which would be a gigantic breaking change, there's little we can do.

A general recommendation would be to:

  1. use plain objects for reactivity and keep classes as nonreactive objects where possible - they manages their own state and side-effects and should not be intertwined with reactivity.
  2. When classes with private fields are used as a property values in a ref or reactive, typecast them when passing them to code expecting an instance of that class as the Unwrapped version is not compatible

Sidenote: Native private fields

the situation is even worse when using native private fields, which will be coming to JS:

class Foo {
  public bar = 1;
  #baz = "a";
}

Those are completely incompatible with Proxies at runtime as well. So instances of such classes need to be marked as raw so that Vue never replaces them with a reactive proxy:

const fooRef = ref(marRaw(new Foo()))

All of this definitely needs propery documentation in vuejs/docs-next.

@LinusBorg LinusBorg changed the title [@vue/reactivity][typing] Ref<T> is not aligned with the return type of ref function [@vue/reactivity][typing] Unwrapping breaks classes with private fields. Jan 10, 2021
@sodatea sodatea added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. need documentation Not necessarily a bug, but proper documentation is needed and removed 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Apr 14, 2023
@BeatsuDev
Copy link

Just want to note that an issue from the ethers.js repository is related to this / depends on this issue:
ethers-io/ethers.js#4131

@LinusBorg
Copy link
Member

I'm inclined to close this issue as there's nothing to fix for us. TS behaves the way it does and we can't work around that. Javascript private fields also seem to be designed not to work with proxies, so there's nothing we can do, really.

@pikax pikax added wontfix This will not be worked on has workaround A workaround has been found to avoid the problem labels Nov 29, 2023
@pikax
Copy link
Member

pikax commented Nov 29, 2023

I'll close this issue because when you assign a value to a Ref it will deep unwrap all the refs, this included in class, meaning the type will be changed!

As a workaround you can define it as a Bail type, that will prevent from specific classes from unwrapping the type, bear in mind the runtime behaviour will be kept the same, use this carefully.

declare module '@vue/reactivity' {
    export interface RefUnwrapBailTypes {
        classes: Foo
    }
}

playground

Other way to solve this issue is marking foo as raw that will also prevent the type from being unwrapped, which at runtime should behave correctly.

let foo: Foo = ref(markRaw(new Foo())).value
// @ts-expect-error .value is not Foo
let fooError: Foo = ref(new Foo()).value

@pikax pikax closed this as completed Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has workaround A workaround has been found to avoid the problem need documentation Not necessarily a bug, but proper documentation is needed scope: reactivity scope: types wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants