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

Make the type of the return value more precise #46

Merged
merged 2 commits into from
Jan 10, 2021
Merged

Make the type of the return value more precise #46

merged 2 commits into from
Jan 10, 2021

Conversation

slaweet
Copy link
Contributor

@slaweet slaweet commented Aug 4, 2020

If the first return value (error) is null, then this change implies that the second return value is not undefined.

It solves a problem I was facing: getting Type 'APIResponse<any> | undefined' is not assignable to type 'APIResponse<any>' type error on the last row of the following Vuex (with vuex-module-decorators) example:

    @Action({ rawError: true })
    public async deleteAsset(asset: Asset): Promise<APIResponse> {
      this.context.commit('setIsLoading', true);
      const [err, response] = await to(deleteAsset(asset));
      this.context.commit('setIsLoading', false);
      if (err) {
        throw err;
      }   
      this.context.commit('setAssetDeleted', asset);
      return response;
    }

If the first return value (error) is null, then this change implies that the second return value is not `undefined`.

It solves a problem I was facing: getting `Type 'APIResponse<any> | undefined' is not assignable to type 'APIResponse<any>'` type error on the last row of the following Vuex (with vuex-module-decorators) example:
```js
    @action({ rawError: true })
    public async deleteAsset(asset: Asset): Promise<APIResponse> {
      this.context.commit('setIsLoading', true);
      const [err, response] = await to<APIResponse, APIError>(deleteAsset(asset));
      this.context.commit('setIsLoading', false);
      if (err) {
        throw err;
      }   
      this.context.commit('setAssetDeleted', asset);
      return response;
    }
```
@ajschmidt8
Copy link

@scopsy, can we get this merged?

@karlhorky
Copy link
Contributor

@scopsy ran into problems with this today - what do you think about merging this?

src/await-to-js.ts Outdated Show resolved Hide resolved
Co-authored-by: Karl Horky <karl.horky@gmail.com>
@scopsy scopsy merged commit 8100f42 into scopsy:master Jan 10, 2021
@karlhorky
Copy link
Contributor

@scopsy nice, thanks for the merge! Will this be released in a new version 2.1.2?

@scopsy
Copy link
Owner

scopsy commented Jan 10, 2021

Hi! Yes this will be released in an new minor version. Hopefully today :)

@slaweet slaweet deleted the patch-1 branch January 11, 2021 06:35
@slaweet
Copy link
Contributor Author

slaweet commented Jan 13, 2021

Hi! Yes this will be released in an new minor version. Hopefully today :)

Great. I'm looking forward to the release.

@mrmartineau
Copy link

FYI, this was released yesterday as v3.0.0

nicholaschiang added a commit to tutorbookapp/tutorbook that referenced this pull request Mar 20, 2021
Adds the changes in scopsy/await-to-js#46 to our local type definitions
(which exist because I'm waiting on scopsy/await-to-js#47).
nicholaschiang added a commit to tutorbookapp/tutorbook that referenced this pull request Mar 20, 2021
Adds the changes in scopsy/await-to-js#46 to our local type definitions
(which exist because I'm waiting on scopsy/await-to-js#47).
nicholaschiang added a commit to tutorbookapp/tutorbook that referenced this pull request Mar 27, 2021
Adds the changes in scopsy/await-to-js#46 to our local type definitions
(which exist because I'm waiting on scopsy/await-to-js#47).
@WilliamStephens
Copy link

WilliamStephens commented Feb 3, 2022

Does this currently work? I'm still having to cast user as UserEntity because the inferred type is
const user: UserEntity | undefined

  createUser = async (userToCreate: Omit<UserEntity, 'id'>): Promise<UserDTO> => {
    const [err, user] = await to(this.UserRepository.createUser(userToCreate));
    if (err) {
      this.Logger.error('Failed to create user.', err.message);
      throw err;
    }

    const userDTO = UserMapper.userEntityToDTO(user as UserEntity);
    return userDTO;
  };

@slaweet
Copy link
Contributor Author

slaweet commented Feb 3, 2022

@WilliamStephens it likely doesn't work. I don't know why. I stopped using await-to-js with typescript because of it.

@karlhorky
Copy link
Contributor

karlhorky commented Feb 3, 2022

If the types of await-to-js are returning a union like {err: Error} | {user: User}, it would work if you don't destructure.

Destructuring makes TypeScript "forget" about the relationship between the two variables err and user, but if you don't destructure, TypeScript can narrow the type correctly:

const results = await to(this.UserRepository.createUser(userToCreate));
if ('err' in results) {
  this.Logger.error('Failed to create user.', results.err.message);
  throw results.err;
}

const userDTO = UserMapper.userEntityToDTO(results.user); // ✅ results.user = User type
return userDTO;

The overarching feature set to allow things like destructuring is described as "Correlated Union Types", which some of the newer (and upcoming) versions of TypeScript support, at least partially:

microsoft/TypeScript#30581

@WilliamStephens
Copy link

WilliamStephens commented Feb 3, 2022

Hmm, yeah seems like destructuring is whats causing it. I converted the return type to be an object to make it a little more explicit.

export function to<T, U = Error>(
  promise: Promise<T>,
  errorExt?: object
): Promise<{ err: U; data: undefined } | { err: null; data: T }> {
  return promise
    .then((data: T) => ({ err: null, data }))
    .catch((err: U) => {
      if (errorExt) {
        const parsedError = Object.assign({}, err, errorExt);
        return { err: parsedError, data: undefined };
      }

      return { err, data: undefined };
    });
}

Then I can do something like this

  createUser = async (userToCreate: Omit<UserEntity, 'id'>): Promise<UserDTO> => {
    const result = await to(this.UserRepository.createUser(userToCreate));
    if (result.err) {
      this.Logger.error('Failed to create user.', result.err.message);
      throw result.err;
    }

    const userDTO = UserMapper.userEntityToDTO(result.data);
    return userDTO;
  };
}

Instead of doing something like if(results[0]) throw results[0].message

Thanks for the replies!

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.

6 participants