fix: path parameters are not encoded #679 #895
Conversation
|
Someone is attempting to deploy a commit to a Personal Account owned by @anymaniax on Vercel. @anymaniax first needs to authorize it. |
|
@Maxim-Mazurok thanks for the help. It's encode every parameters? If wouldn't be better to add that as a config? |
I would argue that it should be encoded by default, for example But I guess it'd be easier to make a non-default option, then no need for major release, I'll do that. |
|
I would think we want all parameters encoded by default no? |
I would want that as well, but as @anymaniax mentioned this will be a breaking change and might require a major release. I think that would be a good idea overall, what do you think? |
|
I mean I guess I don't see the harm in encoding URLs it feels like the safe thing to do. I wonder what @anymaniax concern is? |
Well, I'm pretty sure that we agree that it's a good thing, but changing default behaviour will be a breaking change. For example, if someone had So I think the best thing to do is to encode all params by default and make a major release (major version bump) so that people know to expect breaking changes. The second best thing to do is to make encoding params a non-default option. |
| } | ||
|
|
||
| if (output?.urlEncodeParameters) { | ||
| route = makeRouteSafe(route); |
There was a problem hiding this comment.
This is the core change that addresses the original issue
|
This is ready for review now Summary:
|
| route: string, | ||
| prepend: string, | ||
| append: string, | ||
| ): string => route.replaceAll(/\${(.+?)}/g, `\${${prepend}$1${append}}`); |
There was a problem hiding this comment.
getRouteAsArray uses a slightly different regex
There was a problem hiding this comment.
Ah, interesting. I think mine is better because it seems to be more aligned with OpenAPI specs by allowing special characters and an empty string: OAI/OpenAPI-Specification#1969 (comment)
But yeah, it's probably better to refactor it to avoid duplication
There was a problem hiding this comment.
I've created #1084 to address that
Status
READY
Description
Addresses #679 issue, based on my patch #679 (comment)
Also updates vue to the latest, so that we can use
MaybeReffrom it's indirect dependency@vue/reactivity. I need this because the old import was unresolved when using"moduleResolution": "bundler"which is the default now for vue in innode_modules/@vue/tsconfig/tsconfig.jsonI think some people might have implemented encoding on their end, so this probably should be a major release, otherwise people will get double-encoding.
Todos