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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve PathParams type to support interface #1219

Merged
merged 5 commits into from Apr 27, 2022
Merged

feat: improve PathParams type to support interface #1219

merged 5 commits into from Apr 27, 2022

Conversation

kotarella1110
Copy link
Contributor

@kotarella1110 kotarella1110 commented Apr 26, 2022 •
edited

Passing an interface to the generics of PathParams causes a type error because interface doesn't have an implicit index signature.

interface Params {
  userId: string
}

rest.get<never, Params>('/user', (req) => { // Type 'Params' is not assignable to type 'PathParams'. Index signature for type 'string' is missing in type 'Params'.(2322)
  req.params
})

I would like to resolve this, but for type safety reasons if you cannot approve this PR please close it 馃檹馃徎

@kotarella1110 kotarella1110 marked this pull request as draft April 27, 2022 00:31
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 27, 2022 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 638b92d:

Sandbox Source
MSW React Configuration

@kotarella1110 kotarella1110 marked this pull request as ready for review April 27, 2022 02:51
kettanaito
kettanaito approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Awesome changes, @kotarella1110! 馃帀
I've added some polishing with the latest commit. Now the tests should pass without issues. Let's wait for that CI before we merge.

kettanaito
kettanaito reviewed Apr 27, 2022
View reviewed changes
src/rest.ts
@@ -12,7 +12,7 @@ function createRestHandler<Method extends RESTMethods | RegExp>(
) {
return <
RequestBodyType extends DefaultBodyType = DefaultBodyType,
Params extends PathParams = PathParams,
Params extends PathParams<keyof Params> = PathParams,
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting pattern.
So, if I read this correctly, by accepting key type as a generic, we are able to limit the known keys to the immediate keys of the current Params interface. This way the index signature is not ambiguous and instead is limited to the known keys.

@kettanaito kettanaito merged commit 4f2e851 into mswjs:main Apr 27, 2022
@kettanaito
Copy link
Member

Welcome to contributors! 馃帀

@kotarella1110 kotarella1110 deleted the feat/improve-path-params-type branch April 27, 2022 11:59
kettanaito added a commit that referenced this pull request May 17, 2022
@kotarella1110 @kettanaito
feat: improve PathParams type to support interface (#1219)
b70266f 
* feat: improve PathParams type to support interface

* test: add

* refactor: improve PathParams type

* fix: remove optional from properties of PathParams type

* chore: name generics, move path params test

Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@kettanaito
Copy link
Member

Released: v0.40.0 馃帀

This has been released in v0.40.0!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kettanaito kettanaito kettanaito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kotarella1110 @kettanaito