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

Expanding ComputedGetter's return type #738

Closed
shirakomamu opened this issue Jun 29, 2021 · 4 comments
Closed

Expanding ComputedGetter's return type #738

shirakomamu opened this issue Jun 29, 2021 · 4 comments

Comments

@shirakomamu
Copy link

I am attempting to use a computed value in the setup() block of a component, such as follows:

const store = useStore();

const computedItem = computed({
  get(): string | null {
    return store.getters["getterHere"]; // string, sometimes null
  },
  set(newValue: string): void {
    store.commit("setterHere", newValue);
  },
});

The getter normally returns a string, but as it also can sometimes be null. I therefore annotated the getter's return as string | null. (The value in the store is initialized elsewhere, hence the possibility of the null value.)

However, I'm getting a Vetur error 2769 on the get() due to the return value of null not being assignable to string.

I believe this error occurs due code in the following files:

  1. https://github.com/vuejs/composition-api/blob/b7c7211e560f5966a563dc393ca6f30321a6416b/src/component/componentOptions.ts
  2. https://github.com/vuejs/composition-api/blob/7955e2879754a26db57ca7ef7da35d3098c84480/src/apis/computed.ts

In this code, the type that is used for the argument for ComputedSetter is reused as the return type of the ComputedGetter. So in this case, the TypeScript error occurs because it is attempting to coerce the return type of the get function to string instead of string | null.

Might it be possible to allow the getter function's return type to be a superset containing the type passed into the setter?

@nooooooom
Copy link

Hi, I tried to read the official Vue3 documentation and the implementation of computed in the reactivity system, and found that from Vue2's type declaration to the present, the types of computed The definition is always like this.

Perhaps we should consider the usage scenarios of the code. Usually we don’t write setter for computed, because the method of use can make our purpose of setting values more clear. If you need to limit the types of input parameters, this may be A better way.

More often we will implement the setter on the v-model, but think about it the other way around, since we can provide a parameter of type string | null to the v-model, why do we need to limit his return value The type is string.

So from the perspective of usage scenarios, the official type declaration does not seem to have an impact on usage. Perhaps you can set it up to elaborate on your usage scenarios.

@shirakomamu
Copy link
Author

shirakomamu commented Jun 29, 2021

Hello,

Thank you for your informative reply. I had not considered that the issue would come from Vue's core reactivity system itself, so I apologize if this is not the right place to discuss this request.

The reverse case you bring up (getter of type string, and setter of type string | null) is interesting in that it might not be intuitive at a first glance. However, there might be a case such as where the return type is required to be coalesesced to a string (perhaps for use in a HTML form). In this scenario, a null setter might actually be interpreted to set to an empty string "" instead:

const computedValue = computed({
  get: (): string => {
    return someValue;
  },
  set: (newValue: string | null): void => {
    someValue = newValue || "";
  }
};

For my use case specifically, I am storing a user object in vuex which may either be populated (if the user is logged in) or nulled (if the user is not logged in). In this store is contained the user's preferences, such as privacy settings. These settings are then used to populate the user's preferences page where they are able to modify it through a dropdown with a v-model.

For example, one of the preferences might be "Profile visibility": anyone, friends_only, or private. These are the enumerations possible for this property when selecting a value, but the value in the store may not exist in the first place and is coerced as null in that case.

As the user selects a new value for the preference, the value in the store is updated (and a server call is made to save the new preference).

This preferences page may not realistically be accessible in the case where the user object is null (as the user would not be logged in), but that does not unfortunately prevent the TypeScript error from appearing.

The relevant code (unnecessary cruft removed) is as follows:

const store = getStore();
const user = computed<User | null>(() => store.getters["auth/user"]); // null if the user is not logged in, and User type if logged in

// v-model on a dropdown element
const profileVisibilitySelection = computed({
  get(): ProfileVisibilityEnum | null {
    return user.value?.meta.privacySettings?.profileVisibility || null;
  },
  async set(newValue: ProfileVisibilityEnum): Promise<void> {
    // store old value in case it's needed later
    const oldValue = user.value?.meta.privacySettings?.profileVisibility || null;
    
    // immediately update local data for user experience, rollback later if necessary
    store.commit(
      "auth/setUser",
      Object.assign({}, user.value, {
        meta: {
          ...user.value?.meta,
          privacySettings: {
            ...user.value?.meta.profileVisibility,
            profileVisibility: newValue,
          },
        },
      })
    );

    // send updated value to server to be confirmed
    const serverResponse =
      await axios.request({
        method: "patch",
        url: "/api/auth/me/preferences",
        data: {
          profileVisibility: newValue,
        },
      });

    // rollback the change if server rejected request
    if (!serverResponse.status !== 200) {
      store.commit(
        "auth/setUser",
        Object.assign({}, user.value, {
          meta: {
            ...user.value?.meta,
            privacySettings: {
              ...user.value?.meta.profileVisibility,
              profileVisibility: oldValue,
            },
          },
        })
      );
    }
  },
});

There are currently some workarounds in consideration for my case:

  1. Including null as a parameter type in the setter allows the getter to return a null as well. I do not think this approach is suitable because, at least in my case, null is not actually a value that can be passed into the setter.
async set(newValue: ProfileVisibilityEnum | null): Promise<void> {
  ...
}
  1. Instead of using computed, to use watch instead. In my case, this may be preferable because of the asynchronous operation. I believe that the Vue documentation suggests the usage of watchers for asynchronous/expensive operations, but this scenario I present is not always limited to occurring in asynchronous/expensive operations.
const profileVisibilitySelection = ref<ProfileVisibilityEnum | null>(user.value?.meta.privacySettings?.profileVisibility || null);
watch(
  profileVisibilitySelection,
  () => {
    // update store and perform asynchronous operation here
  }
);
  1. Setting a default value in the case of null, so that the getter never returns null. I do not think this approach is suitable because in some cases, a non-option may need to be differentiated from a default option, such as when introducing new preferences where the user must explicitly choose a value instead of having a default one chosen for them.
get(): ProfileVisibilityEnum {
  return user.value?.meta.privacySettings?.profileVisibility || ProfileVisibilityEnum.private;
},
  1. Assert the value returned by the getter to be non-null. I believe this is usable in my exact scenario only because the preferences page is realistically not accessible when it is null. However, in a scenario where the null is visible to the user (such as that presented in point 3 above), the assertion would not be valid.
get(): ProfileVisibilityEnum {
  return user.value?.meta.privacySettings?.profileVisibility as ProfileVisibilityEnum;
},

@nooooooom
Copy link

Hello,
Thank you for understanding what I mean, my English is not very good. Thank you for taking the time to describe the problem you encountered and use your code as an example.

Sorry for taking so long to reply to you. After reading your case and your own thinking about the solution, I think case one is the main way to solve your problem.

Let’s talk about type definition first (this is our main issue). In the description of computed in the official Vue documentation, getter is more like the formatting we define, and setter is more like the analysis of formatted values. And assign the analysis results to other values. So in order for the setter to take effect correctly, we keep the type of the getter consistent with the parameter type of the setter.

In your business code, my understanding is that you will immediately respond to the user's actions (although there will be some side effects after the value is changed in a certain place), and then use the results returned to you in the background to determine whether you need to respond to the user's Operation (otherwise the value is rolled back to before the corresponding).

From the perspective of user operations, I may receive up to three status changes.

  1. The entered value takes effect successfully (some other view changes may occur)
  2. The client sends a request (may display a loading component) ->
  3. If the request fails, the value I entered will be rolled back and the previous display will beClear.
    This may cause confusion to users, because if my internet speed is not able to respond quickly, the interval between these three states will be infinitely magnified.

If the value is changed from the user, that is, when the setter is running, we will judge and format the input value, and feedback the final result to the user and only make a correct commit once. For users, this result is acceptable. When I choose an option from a bunch of options, you should tell me whether my choice is correct, instead of letting me bear the consequences of switching back and forth. For us, we can correctly parse the value passed in by the setter, intercept it and alert the user before the request (maybe the input format does not meet expectations).

So in this issue, I think the definition of the computed type is in line with the expected effect given by Vue to computed. Getter and setter are like formatter and parser, and they should be related by some kind of connection. For the realization of business code, I think we have the ability to make TypeScript display correctly.

@shirakomamu
Copy link
Author

Hello,

Thank you for your reply and valuable insight. Please do not worry about your English. It was perfectly understandable.

Your wonderful analogy of the getter/setter being akin to formatter/parser had caused me to rethink my approach to this problem.

You are correct that it was my intention to immediately respond to the user's actions. It was my design intention such that every change on the user's side could be done atomically without the need for a "Save changes" button. I had set up an indicator component to inform the user that the asynchronous call is in progress and a success/fail component at its conclusion.

After thinking over your comments, it is clear that I had made an oversight in the case of poor internet connection. As you describe, the user experience will be severely degraded as the server calls may take a long time or fail altogether. From this possibility, I think it is clear to me now that my approach to this specific problem needs to be reconsidered. Please consider my specific business case to be resolved now.

As for the more general use cases for the expansion or separation of types between the getter and setter, I believe there may still be some niche scenarios where it may be useful (particularly around "", undefined, or null). However, as my specific use case for this has been resolved, it may be better for another developer who can make a stronger case for this to do so. Therefore, I will consider this issue to be closed.

Thank you for your time to discuss this issue together. It is very much appreciated.

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

No branches or pull requests

2 participants