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

types: export RefSymbol #3317

Closed
wants to merge 1 commit into from
Closed

Conversation

viniciuskneves
Copy link

If you decide to use readonly from vue you will face the following issue:

image

The issue is that RefSymbol is not exported.

Another example using it directly inside setup:

image

@LinusBorg
Copy link
Member

Could you provide a complete code sample? a screenshot where the code is covered in large parts is hard to reproduce. A complete reproduction in issue would be even better.

I ask because I regularly use readonly and haven't experienced this issue.

@LinusBorg LinusBorg added the need more info Further information is requested label Feb 27, 2021
@Mister-Hope
Copy link

Mister-Hope commented Mar 2, 2021

Could you provide a complete code sample? a screenshot where the code is covered in large parts is hard to reproduce. A complete reproduction in issue would be even better.

I ask because I regularly use readonly and haven't experienced this issue.

If you are trying to export a readonly value from a component, you will get an issue:

setup(){
    const a = readonly(ref(1));

    return {
      a,
    };
  },

I am getting this error because I want to make a composable exporting a readonly value(it 's reactive but should only be modified from the composable side, not in components), and it should bind to the template. @LinusBorg

image

@Mister-Hope
Copy link

image

Full code logic here, useA is a composable.

<script lang="ts">
import { DeepReadonly, defineComponent, readonly, Ref, ref } from "vue";

interface AResult {
  a: DeepReadonly<Ref<number>>;
  updateA: (val: number) => void;
}

const useA = (): AResult => {
  const a = ref(1);

  const updateA = (val: number): void => {
    a.value = val;
  };

  return {
    a: readonly(a),
    updateA,
  };
};

export default defineComponent({
  setup() {
    const { a, updateA } = useA();

    return {
      a,
      updateA,
    };
  },
});
</script>

@LinusBorg
Copy link
Member

Hm, this problem only happens with Vetur, not Volar. Does it break on build? Haven't had time to check that out.

Anyway, I would suspect the issue is rather with DeepReadonly, as it breaks the Ref type and turns it into a normal object type:

https://github.com/vuejs/vue-next/blob/fbf2d68bbcde75c95363e1fd17946c720d0f3925/packages/reactivity/src/reactive.ts#L130-L131

If we added a check for like this before the generic object one, this might be fixed without exposing the internal symbol.

Will try and come up with a fix for that so that we don't have to expose this internal Symbol type.

@LinusBorg LinusBorg self-assigned this Mar 9, 2021
@viniciuskneves
Copy link
Author

viniciuskneves commented Mar 12, 2021

Hey, sorry for getting back late here, I was not available in the past ~2 weeks.

I've setup a branch in the project using readonly: https://github.com/viniciuskneves/vue-snakke/compare/typescript-readonly

I want to run: npm run build:types to generate my declarations.
If I run it in the above branch, I get the following:

src/services/Snakke.ts:3:25 - error TS4058: Return type of exported function has or is using name 'RefSymbol' from external module "/Users/viniciuskneves/workspace/own/vue-snakke/node_modules/@vue/reactivity/dist/reactivity" but cannot be named.

3 export default function useSnakke() {
~~~~~~~~~

Found 1 error.

If I remove it (main branch), I get the declarations as expected.

By the end it is not just an IDE/plugin issue, I think.

Does it help?

@viniciuskneves
Copy link
Author

Hey @LinusBorg, did my last message helped somehow?

@LinusBorg
Copy link
Member

Hey, thanks for the gentle reminder. Need to get back to this, will look at your branch this week as I have the week off. If that branch helps me to reproduce the error it will definitely help.

Already played with a solution but was not sure how to to add a proper test for it without a way to make it fail before ...

@HcySunYang
Copy link
Member

HcySunYang commented May 10, 2021

Hey @viniciuskneves, thanks for this PR, but I will close it because #2548 precedes this PR

@HcySunYang HcySunYang closed this May 10, 2021
@viniciuskneves viniciuskneves deleted the patch-1 branch May 10, 2021 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants