Branding Ref so types with a value property aren't incorrectly unwrapped.

[blainehansen]

I'm running into the problem where this type declared in my codebase:

export type GenericOption<T> = { name: string, value: T }

incorrectly extends Ref, which means that UnwrapRef is unwrapping it to T which isn't correct.

Here's a playground demonstrating this:
https://www.typescriptlang.org/play/?ssl=22&ssc=1&pln=22&pc=63#code/MYewdgzgLgBA+gJwKYDMBCCCGYAmAuGAVzAEsBHQpGCATwFsAjEAGxgF4YBlep5gCgCUAKCg0ADlUSoM2HOxiiJIFPGTosuIUgAeYkAlgkwUJAhSZgVAEqoAPABUAfDADeQmB5gB6LzGSYccGYaVWkNfFD1WQBud08AN0xmSgJ7WIBfISEjEzMLa1QAeTEoEnAHZzcPAHMkKEFU2I8IOoB+Pm1UgQJ4kBIcDKFgZkwICBgbFABJOjFmCpgSWeYkOiRjccmFqr8kAKCQqSjcAiOZXHkz8KaYMUIGZhJgGETkpABCRrjQSCgEQmAUH0fBcMFqUAANNQ6jB0gRJsVSuUnAJXHEPFAABYIEAAdxgYCQ+IAoggcQhBHFMpktLp9LBFFQAOLrUxPRFlMALDigsCYNYEaAIIzVKGvFIweywrKMyVIaDyFmE4XADnlbA0Zw6Ey4TZ2DXOVoKf5UAjmZgtIA

If you hover over the Test type it displays as true right now, but uncommenting the // readonly _refBrand: _refBrand; line will make it correctly display as false.

I'm assuming that since isRef simply does an instanceof check that this is safe and won't interfere with the actual unwrapping algorithm, but I'll admit I haven't deeply looked.

I'm looking forward to Vue 3 coming out! I've enjoyed working with this library so far.

fixes #157
fixes #159

[LinusBorg]

Maybe we can use the symbol as the property key and make it non-enumerable with Object.defineProperty()?

[blainehansen]

I'm having trouble understanding :) Here's what I'm guessing you're saying, but I'm probably guessing wrong since I don't understand this codebase very well.

Change the Ref interface and RefImpl class to look roughly like this:

const _refBrand: unique symbol = Symbol();
type _refBrand = typeof _refBrand;
export interface Ref<T> {
  // readonly _refBrand: _refBrand;
  // value: T;
  [_refBrand]: T;
}

class RefImpl<T> implements Ref<T> {
  // readonly _refBrand: _refBrand = _refBrand;
  // public value!: T;
  public [_refBrand]!: T;
  constructor({ get, set }: RefOption<T>) {
    // proxy(this, 'value', {
    proxy(this, _refBrand, {
      get,
      set,
    });
  }
}

I'm not sure how to make this consistent with the rest of the code. RefImpl is using that proxy method which is defined like this:

const sharedPropertyDefinition = {
  enumerable: true,
  configurable: true,
  get: noopFn,
  set: noopFn,
};

export function proxy(target: any, key: string, { get, set }: { get?: Function; set?: Function }) {
  sharedPropertyDefinition.get = get || noopFn;
  sharedPropertyDefinition.set = set || noopFn;
  Object.defineProperty(target, key, sharedPropertyDefinition);
}

I'm hesitant to change that enumerable: true setting, but really I think I'm just overall just confused and need some more guidance about what you mean ha. I'm guessing you have some internal constraints in your head that I'm not aware of :)

For context, making those Ref and RefImpl changes caused a lot of compile errors like this, so I'm sure I've just misunderstood you:

src/setup.ts:61:18 - error TS2339: Property 'value' does not exist on type 'Ref<unknown>'.

    61       setupValue.value = refs[key];
                        ~~~~~

Looking forward to hearing from you!

[LinusBorg]

That's not quite what I mean. the value property should stay as it is. I was just pondering wether we can make the additional property that you added to be non-enumerable and use the Symbol as a key (and simple true as its value).

class RefImpl<T> {
  private [_refBrand]: true // never tried this in a TS, class, unsure if it works
  public value: T
  constructor() {
    // we probably would have to add it with Object.defineProperty() ?
    Object.defineProperty(this, _refBrand, {
      enumerable: false,
      value: true
    })
  }
}

The reason for that is so that "_refBrand" doesn't appear when inspecting the list of properties that the ref object has, e.g. in (Object.keys). It should be an implementation detail mostly "hidden" from the consumer of the ref object.

I'll see if I can come up with a better sample later, really have to hurry out of the door now.

[blainehansen]

Ahh that makes way more sense ha. I made those changes and the tests passed locally :) Thanks for the guidance!

[blainehansen]

Unfortunately I couldn't make the property private or protected though. Typescript doesn't allow those modifiers in interfaces.

https://www.typescriptlang.org/play/#code/MYewdgzgLgBA+gJwKYDMBCCCGYAmAuGAVzAEsBHQpGCATwFsAjEAGxgF4YBlep5gCgCUAKCQAPAA4gEsEmChIEKTMCoAlVAB4AKgD4YAbyExjMcQhDzg8nDADaiVBmw4AugSgJKAbiMmAbpjMlARaPgC+QkKy8orKaqgA8uJQJODaeobGAOZIUIIhPsYQuQD8fKIhAgR+ICQ44ULAzJgQEDDqKACSdOLM6TAkPcxIdEhybR39mabmltZ2DuhYuC4AhO6eSIWmhAzMJMAwAUFI6zChvqCQHoRWUnz6MDlQADTUuTBhBB1JKWm6AgMvmMUAAFuYAO4wMBIKEAUQQ5gQgl8EQiIgkUlgUBo4ioAHExgoDr9UmB+hxHmBMKMCNAELIsm9jsFzp9Iji8eckNB2DBCTCGcBSWlsDQ9GJ5LgJpoxXoSjAblQCEpmMUgA

[Zero2key]

declare const _refBrand: unique symbol;

export interface Ref<T> {
  value: T;
  [_refBrand]: true
}

class RefImpl<T> implements Ref<T> {
  public value!: T;
  [_refBrand]!: true;
  constructor({ get, set }: RefOption<T>) {
    proxy(this, 'value', {
      get,
      set,
    });
  }
}

just _refBrand is a symbol, not _refBrand is A real Symbol, can fix ts type error

[blainehansen]

Any thoughts about merging this? Has this been fixed in subsequent commits?

[aztalbot]

I was just about to make a PR for these same changes and then found this. Subsequent commits have not fixed this issue. It would be great if this can get merged in and released soon. The unwrapping issue is quite annoying.

[LinusBorg]

/ping @ktsn

[LinusBorg]

@blainhansen can you make the changes that @ktsn proposed?

Thanks!

[ktsn]

LGTM 👍

[LinusBorg]

I'll run this through some manual tests and then merge it.