feat(apigatewayv2): websocket api #13031
Conversation
|
|
github-actions
bot
added
the
@aws-cdk/aws-apigatewayv2
nija-at
changed the title
nija-at
changed the title
There was a problem hiding this comment.
This is awesome. Thanks for submitting this PR @ayush987goyal.
Some very early feedback. Happy to discuss some of these over chat or on a call, if you prefer.
|
@nija-at I have tried to address most of the comments and taken into account pointers from our discussion. The PR has a lot more number of files now but I believe that will probably be well worth it in the long run. |
|
A couple of extra things to note here are these lint errors which I had to exclude but couldn't find proper way to address. Would love some inputs on them. |
There was a problem hiding this comment.
I've reviewed the refactors and given the websocket constructs a quick review, but have not yet reviewed the integration classes and module.
These comments should hopefully give you some directions.
This PR will require a few iterations to get right, so please bear with me.
Some experimental interfaces are renamed
You will need to document all of the breaking changes individually, unfortunately. And it will have to be in this particular format - https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#step-4-commit. This is because we use conventional commits to parse commit messages and generate our CHANGELOG.
It's fine if you wait until all the major review iterations are done before fixing this up.
mergify
bot
dismissed
nija-at’s stale review
Pull request has been modified.
packages/@aws-cdk/aws-apigatewayv2/lib/common/private/integration-cache.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/common/private/integration-cache.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/common/private/integration-cache.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Just one comment. This is close to being shipped.
Can we capture the final set of breaking changes per my comment here - #13031 (review)
For class name changes, just capturing them as a single change is sufficient, but we should capture most if not all property changes in existing classes.
I've found jsii-diff helps to produce this. You should be able to run npx jsii-diff npm: and see the relevant output.
packages/@aws-cdk/aws-apigatewayv2-integrations/test/websocket/integ.lambda.ts
Outdated
Show resolved
Hide resolved
| let stage = props.stage; | ||
| if (!stage) { | ||
| if (props.api instanceof HttpApi) { | ||
| if (props.api.defaultStage) { | ||
| stage = props.api.defaultStage; | ||
| } else { | ||
| throw new Error('stage is required if default stage is not available'); | ||
| } | ||
| } else { | ||
| throw new Error('stage is required for WebSocket API'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't it cleaner to make props.stage mandatory and drop props.api?
There was a problem hiding this comment.
So the api prop would still be required since currently api is not available on IStage since we moved it to subclass IHttpStage and IWebSocketStage. Now given that, we can keep this implementation because it provided convenience for HttpApi users (though agree this is not the cleanest of solutions but maybe in future it can be clear when we have defaultStage for WS apis as well). Thoughts?
There was a problem hiding this comment.
So now we can go from IHttpStage -> IHttpApi but can't go from IStage -> IApi? Whoops.
I think we should somehow get both but let's not try to solve this now in this PR.
Leave it as such.
feat(apigatewayv2): add support for WebSocket APIs BREAKING CHANGE: `HttpApiMapping` (and related interfaces for `Attributed` and `Props`) has been renamed to `ApiMapping` * **apigatewayv2:** `CommonStageOptions` has been renamed to `StageOptions` * **apigatewayv2:** `HttpStage.fromStageName` has been removed in favour of `HttpStage.fromHttpStageAttributes` * **apigatewayv2:** `DefaultDomainMappingOptions` has been removed in favour of `DomainMappingOptions` * **apigatewayv2:** `HttpApiProps.defaultDomainMapping` has been changed from `DefaultDomainMappingOptions` to `DomainMappingOptions` * **apigatewayv2:** `HttpApi.defaultStage` has been changed from `HttpStage` to `IStage` * **apigatewayv2:** `IHttpApi.defaultStage` has been removed closes aws#2872 Some notes: 1. Only Lambda Integration is currently supported 2. No support for `IntegrationResponse` and `RouteResponse`. 3. The `$default` stageName does not seem to work for WebSocket APIs. Therefore modified the API for defaultStage in the API. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
feat(apigatewayv2): add support for WebSocket APIs
BREAKING CHANGE:
HttpApiMapping(and related interfaces forAttributedandProps) has been renamed toApiMappingCommonStageOptionshas been renamed toStageOptionsHttpStage.fromStageNamehas been removed in favour ofHttpStage.fromHttpStageAttributesDefaultDomainMappingOptionshas been removed in favour ofDomainMappingOptionsHttpApiProps.defaultDomainMappinghas been changed fromDefaultDomainMappingOptionstoDomainMappingOptionsHttpApi.defaultStagehas been changed fromHttpStagetoIStageIHttpApi.defaultStagehas been removedcloses #2872
Some notes:
IntegrationResponseandRouteResponse.$defaultstageName does not seem to work for WebSocket APIs. Therefore modified the API for defaultStage in the API.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license