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

Add return type to rememo jsDoc type annotations #7

Closed
wants to merge 1 commit into from

Conversation

adamziel
Copy link

What problem does this PR solve?

Right now, the createSelector function loses the selector type details:

const iReturnNumbers = createSelector(
	( stringArg: string ) => 123,
	() => []
);
const iShouldBeANumber = iReturnNumbers();
// iShouldBeANumber is of type `any`

This is a problem in Gutenberg which depends on this package. For example, see the following PR: WordPress/gutenberg#41235 (comment)

How does this PR propose to solve this problem?

Adding the following @return type annotation preserves the selector type signature:

 * @return {S} Memoized selector.

The above snippet now works as expected:

const iReturnNumbers = createSelector(
	( stringArg: string ) => 123,
	() => []
);
const iShouldBeANumber = iReturnNumbers();
// iShouldBeANumber is of type `number`

Test plan

This changes just the documentation string, there are no runtime changes to test. Eyeballing the changes and confirming the types are now resolved as in the above examples should suffice.

Caveat: this could be considered a breaking change for any projects using createSelector in TypeScript. After upgrading rememo to a patched version, the selectors will be of a different type, which could cause type errors. This could be addressed by releasing a new major point release.

cc @aduth @dmsnell

## What problem does this PR solve?

Right now, the `createSelector` function loses the selector type details:

```ts
const iReturnNumbers = createSelector(
	( stringArg: string ) => 123,
	() => []
);
const iShouldBeANumber = iReturnNumbers();
// iShouldBeANumber is of type `any`
```

This is a problem in Gutenberg which depends on this package. For example, see the following PR: WordPress/gutenberg#41235 (comment)

## How does this PR propose to solve this problem?

Adding the following `@return` type annotation preserves the selector type signature:

```
 * @return {S} Memoized selector.
```

The above snippet now works as expected:

```ts
const iReturnNumbers = createSelector(
	( stringArg: string ) => 123,
	() => []
);
const iShouldBeANumber = iReturnNumbers();
// iShouldBeANumber is of type `number`
```

## Test plan

This changes just the documentation string, there are no runtime changes to test. Eyeballing the changes and confirming the types are now resolved as in the above examples should suffice.

Caveat: this could be considered a breaking change for any projects using `createSelector` in TypeScript. After upgrading rememo to a patched version, the selectors will be of a different type, which could cause type errors. This could be addressed by releasing a new major point release.

cc @aduth @dmsnell
@aduth
Copy link
Owner

aduth commented May 27, 2022

Hey @adamziel , thanks for submitting this. It was expected that the return type would have been inferred by the JSDoc cast in the function return statement, so rather odd that it's falling back to the any type.

return /** @type {S & EnhancedSelector} */ (callSelector);

Also, the return type isn't strictly the same type as the original function, since it also supports the clear and getDependants function properties. I believe Gutenberg uses both of these functions as well, so losing that type detail might cause other breakage.

I do see in the published package's .d.ts declaration that the return type is S & EnhancedSelector (see line 16), so it looks like TypeScript is at least picking up that much correctly from the JSDoc. Maybe it's something with the intersection?

Not sure if you'd be willing to take a look into this, otherwise I can plan to spend some more time tomorrow evening digging into it.

@aduth
Copy link
Owner

aduth commented May 27, 2022

I'm having trouble reproducing the original issue. I created a CodeSandbox, but it appears to be picking up the return value just fine with the given example?

https://codesandbox.io/s/wonderful-wave-3qtrbi?file=/src/index.ts

image

@adamziel
Copy link
Author

@aduth that's a great news! To close the loop here, I just merged your Gutenberg PR WordPress/gutenberg#41415. I won't be able to fully check whether the types do what we need them to until after the WordCamp EU, but all the signs seem to say that upgrading rememo to a newer version should do the trick so I'm going to close this PR now. Thank you once gain!

@adamziel adamziel closed this May 28, 2022
@aduth
Copy link
Owner

aduth commented May 31, 2022

Cool, hope it all works for you. Have a great time at WordCamp!

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.

2 participants