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: fixed typescript error dynamic paths #2106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

walid-mos
Copy link

Changes

Openapi-typescript creates an faulty file when using --path-params-as-types to generate types when nested path parameters exists.

This is particularly annoying when used with a Restful CRUD like :

  • /users
  • /users/{userId}
    ...

#1752

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

  • [ x] Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@walid-mos walid-mos requested a review from a team as a code owner January 20, 2025 20:32
@walid-mos walid-mos requested a review from gzm0 January 20, 2025 20:32
Copy link

netlify bot commented Jan 20, 2025

👷 Deploy request for openapi-ts pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit abbea8c

Copy link

changeset-bot bot commented Jan 20, 2025

⚠️ No Changeset found

Latest commit: abbea8c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@walid-mos walid-mos marked this pull request as draft January 20, 2025 21:59
@walid-mos walid-mos marked this pull request as ready for review January 20, 2025 22:07
@walid-mos
Copy link
Author

Hello, I drafted the PR because I thought there was a bug in my solution.

Finally I came across this observation, when you use a path that has a static part after the dynamic part, you have to add a / to make the typing work.

I don't know if that's likely to cause a problem, in my case it's fine, but I don't want to speak for other people.

Here's some example of what i am talking about :
Capture d’écran 2025-01-20 à 23 07 06
Capture d’écran 2025-01-20 à 23 10 20

without trailing slash:
Capture d’écran 2025-01-20 à 23 10 43

any other path works fine

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Hi @walid-mos, thanks a lot for contributing!

I have some questions / concerns about the overall approach.

I have posted them on the tracking issue:
#1752 (comment)

Let's discuss there!

@duncanbeevers duncanbeevers added the openapi-ts Relevant to the openapi-typescript library label Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openapi-ts Relevant to the openapi-typescript library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants