-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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(common/core): api versioning #6278
Conversation
Pull Request Test Coverage Report for Build 1932f99e-e7fd-4a1d-b34f-1b86ce4022ae
💛 - Coveralls |
/** | ||
* The name of the Request Header that contains the version. | ||
*/ | ||
header: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to provide a default value here - like X-API-Version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would hurt. Personally if I were use the custom header functionality, I'd probably specify the header anyways just so people reading the code later on would be aware of it.
*/ | ||
export const VERSION_NEUTRAL = Symbol('VERSION_NEUTRAL'); | ||
|
||
export type VersionValue = string | string[] | typeof VERSION_NEUTRAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add (string) => boolean
. This is useful if we do semver where the major version is the controller's version, and the minor version is the route's version.
For example:
@Controller({version: v => semver.satisfies(v, '2.x')})
class MyController {
@Version(v => semver.satisfies(v, '>=2.5.1'))
@Get(':id')
newGetter();
@Version(v => semver.satisfies(v, '>=2.2.0 <=2.5.0'))
@Get(':id')
previousGetter();
@Version(v => semver.satisfies(v, '<2.2.0'))
@Get(':id')
oldGetter();
}
That way we don't have to edit the @Version
decorators of each versioned route of every controller every time we add a new version for a specific route of a specific controller.
Actually it would be amazing if semver was supported out of the box - maybe by adding an option to enableVersioning
.
We could just do then:
@Controller({version: '2.x'})
class MyController {
@Version('>=2.5.1')
@Get(':id')
newGetter();
@Version('2.2.0 - 2.5.0'))
@Get(':id')
previousGetter();
@Version('<2.2.0')
@Get(':id')
oldGetter();
}
or even:
@Controller({version: '2'})
class MyController {
@Version('2.5.1')
@Get(':id')
newGetter();
@Version('2.2.0')
@Get(':id')
previousGetter();
@Get(':id')
oldGetter();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of that, but I'd have get @kamilmysliwiec's opinion since it would be adding a new prod dependency.
I also wouldn't want to hold this PR up while that is being worked on, so I'd probably have it as a fast-follower enhancement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semver
looks like a very lightweight dependency so I think adding it to the framework's deps should be fine.
I also wouldn't want to hold this PR up while that is being worked on, so I'd probably have it as a fast-follower enhancement
Would it require a lot of effort to add this to the current PR? If not, I'd appreciate it a lot if we could proceed with this feature as part of this pull request 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh your blessing was why I was hedging my bets, but your quick reply proved me wrong! I'll go ahead and add this in 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've got semver working for Header & Media Type versioning. But adding it for URI versioning isn't as simple unfortunately.
So for Header and Media Type, it's able to work because the URI route is static and the version filter runs when the request is received.
But URI does it differently because of the way that the router works. TLDR the router stores the static URLs (presumably so it can easily send a req to the handler or send a 404), and any route that isn't in that store will get a 404. I accommodated for that in the original design by adding a new route for the specific versions, so /v1/route1
and /v2/route2
would be their own routes according to the router. Because semver would accept a variety of ranges, this pattern won't work with it.
An alternative (which I don't like), would be to have the version be a parameter like /:version/route1
. This would allow us to dynamically determine that version value and ultimately use semver for it before invoking the route handler. But I really don't like the idea of having this hidden parameter. Even if it's named something that is unlikely to collide with a consumer set parameter, it just seems like a code smell to me.
But the alternative is not having semver for URL versioning at all. So i'd love some other's opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the alternative is not having semver for URL versioning at all.
What worries me, in this case, is that having a semver
support only for Header & Media Type would very likely introduce a lot of confusion in the community (as it's quite an important inconsistency). TBH I don't see any viable solution to this that wouldn't lead to performance downgrade + fuss atm. We may need to drop this idea for the time being and maybe, reconsider it in the future under a flag (it would have to be turned on explicitly + we would have to warn ppl in the docs that enabling this functionality have some significant cons).
An alternative (which I don't like), would be to have the version be a parameter like /:version/route1. This would allow us to dynamically determine that version value and ultimately use semver for it before invoking the route handler.
I personally dislike it as well since it will affect the overall performance too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. I pushed the code I had to a branch for reference down the line.
@rich-w-lee Thank you, great work! Is it possible to also support querystring-based versioning? Like |
@constb I personally am not a fan of query string versioning because of possible query parameter collision, so I didn't focus on including that (plus the fact that the other 3 use cases seem more popular). If there is enough interest in it, I'm not opposed to adding it in, but like the above (semver), I wouldn't want to hold this PR up while that is being worked on, so I'd do it as a fast-follower enhancement |
544aa06
to
ba60e4a
Compare
Thanks for your contribution @rich-w-lee! I'll review it as soon as possible |
chore(deps): update dependency husky to v5
…-monorepo chore(deps): update typescript-eslint monorepo to v4.15.0
fix(deps): update dependency typeorm to v0.2.31
chore(deps): update dependency ts-loader to v8.0.16
…cavb/nest into lucavb-feature/pipes-fileinterceptor
…anager-3.x chore(deps): update dependency amqp-connection-manager to v3.2.2
adding the ability to have different versions of routes to support changing applications that still need to support legacy consumers
closes #5065
PR Checklist
Please check if your PR fulfills the following requirements:
^ The docs for this feature are in progress, waiting for review from maintainer to ensure the interface is to their standards
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #5065
What is the new behavior?
See the above issue.
Does this PR introduce a breaking change?
Other information
As mentioned above, the docs for this feature are in progress, I'd like a review from maintainer to ensure the interface is to their standards before I push that out.