-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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(websockets): add prefix option for api versioning #10597
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build efd3d236-d3d5-498c-b8d8-1f7f949aa4d5
💛 - Coveralls |
options: TOptions, | ||
) { | ||
const { path = '', prefix = '', ...passthrough } = options; | ||
console.log(options); |
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.
console.log(options); |
The console.log()
should be removed
0b84258
to
b683ae9
Compare
* Api version acts as prefix to path | ||
* @default undefined | ||
*/ | ||
prefix?: 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.
since we have prefix and versioning for @Controller()
, I found "using prefix
for api versioning" a bit strange
Can't we avoid mention that specific use case? 🤔 This leaves room for adding first-class versioning in the future
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.
do you mean leave the description as just "path prefix"?
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.
yep
@@ -27,16 +27,21 @@ describe('@WebSocketGateway', () => { | |||
expect(port).to.be.eql(0); | |||
}); | |||
|
|||
@WebSocketGateway({ namespace: '/' }) | |||
@WebSocketGateway({ namespace: '/', path: '/path', prefix: '/prefix' }) |
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.
can we add another test solely to check if the prefix is being added, instead of replacing this one that has no prefix? so we will test both versions
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
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Websocket gateway options has only
path
and no 'prefix'Issue Number: (#9376)
What is the new behavior?
websocket gateway options now has
path
andprefix
which get joined for the full path. this can be used for api versioningDoes this PR introduce a breaking change?
Other information