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

feat(types): remove this binding expectations on hook fn types #736

Merged
merged 2 commits into from Jan 24, 2022

Conversation

bilalq
Copy link
Contributor

@bilalq bilalq commented Jan 14, 2022

Summary

This change improves the experience for TypeScript users that have the unbound-method eslint rule enabled. Without this change, check failures occur when users attempt to use destructured references to the functions returned in the useAsyncStorage hook's envelope object.

Example:

const { getItem, setItem } = useAsyncStorage('someKey')

// eslint@typescript-eslint/unbound-method
//
// Avoid referencing unbound methods which may cause unintentional
// scoping of `this`. If your function does not access `this`, you can
// annotate it with `this: void`, or consider using an arrow function
// instead.

Test Plan

This is a type change only, so no direct testing is applicable. However, the impacts of this typing change can be explored/tested in this TypeScript playground here.

This change uses a feature supported even in really old versions of TypeScript, so it should not be considered a breaking change. Even the oldest version available in the TypeScript playground supports this change just fine.

This change improves the experience for TypeScript users that have the
[`unbound-method`](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/unbound-method.md)
rule enabled. Without this change, check failures occur when users
attempt to use destructured references to the functions returned in the
`useAsyncStorage` hook's envelope object.

Example:

```typescript
const { getItem, setItem } = useAsyncStorage('someKey')

// eslint@typescript-eslint/unbound-method
//
// Avoid referencing unbound methods which may cause unintentional
// scoping of `this`. If your function does not access `this`, you can
// annotate it with `this: void`, or consider using an arrow function
// instead.
```
types/index.d.ts Outdated Show resolved Hide resolved
This approach achieves the same end result as explicitly declaring the `this` type to be void and is more familiar to readers who know JS.

Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@bilalq bilalq changed the title feat(types): set void as this type of hook fns feat(types): remove this binding expectations on hook fn types Jan 21, 2022
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. ❤️

@krizzu: Please merge when you've had the chance to review. This should probably be a fix(types): so we don't bump minor version.

@krizzu krizzu merged commit cf76a4e into react-native-async-storage:master Jan 24, 2022
@krizzu
Copy link
Member

krizzu commented Jan 24, 2022

Thanks for the fix ❤️

@krizzu
Copy link
Member

krizzu commented Jan 24, 2022

🎉 This PR is included in version 1.15.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants