Skip to content

Improvement suggestion for Middleware type definition #1916

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

Closed
1 task done
yicrotkd opened this issue Sep 20, 2024 · 3 comments · Fixed by #1918
Closed
1 task done

Improvement suggestion for Middleware type definition #1916

yicrotkd opened this issue Sep 20, 2024 · 3 comments · Fixed by #1918
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library

Comments

@yicrotkd
Copy link
Contributor

yicrotkd commented Sep 20, 2024

Description

The middleware throws the following error when neither onRequest() nor onResponse() is provided.

"Middleware must be an object with one of onRequest() or onResponse()"
https://github.com/openapi-ts/openapi-typescript/blob/main/packages/openapi-fetch/src/index.js#L223

Based on this, I think the type definition below could be improved by making it more explicit and better aligned with the intended behavior: https://github.com/openapi-ts/openapi-typescript/blob/main/packages/openapi-fetch/src/index.d.ts#L147

Proposal

It seems like the Middleware interface might be improved with the following modification:

export type Middleware {
  onRequest?: ...;
  onResponse: ...;
} | {
  onRequest: ...;
  onResponse?: ...;
}

This modification ensures that either onRequest or onResponse is required, and this is reflected in the type, allowing the issue to be detected at compile time.

Checklist

@yicrotkd yicrotkd added enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library labels Sep 20, 2024
@drwpow
Copy link
Contributor

drwpow commented Sep 20, 2024

Good suggestion! I’d be open to a PR for this

@yicrotkd
Copy link
Contributor Author

Thank you for the quick response! I’m glad the suggestion makes sense. Just to clarify, I was actually planning to open a PR for this myself, as mentioned in the checklist. I’ll go ahead and submit it shortly.

If possible, I’d really appreciate your feedback once the PR is open!

@yicrotkd
Copy link
Contributor Author

I've created the PR.

#1918

I would appreciate it if you could review it when you have the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants