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(apigatewayv2): websocket api #13031

Merged
merged 15 commits into from Mar 5, 2021
Merged

feat(apigatewayv2): websocket api #13031

merged 15 commits into from Mar 5, 2021

Conversation

@ayush987goyal
Copy link
Contributor

@ayush987goyal ayush987goyal commented Feb 13, 2021

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 #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

@gitpod-io
Copy link

@gitpod-io gitpod-io bot commented Feb 13, 2021

@nija-at nija-at changed the title feat(apigatewayv2): add support for WebSocket APIs feat(apigatewayv2): websocket apis Feb 16, 2021
@nija-at nija-at changed the title feat(apigatewayv2): websocket apis feat(apigatewayv2): websocket api Feb 16, 2021
Copy link
Contributor

@nija-at nija-at left a 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.

packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
@ayush987goyal ayush987goyal force-pushed the ayush987goyal:pr/ws branch from 36f5feb to 0158d44 Feb 19, 2021
@ayush987goyal ayush987goyal requested a review from nija-at Feb 19, 2021
@ayush987goyal
Copy link
Contributor Author

@ayush987goyal ayush987goyal commented Feb 19, 2021

@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.

@ayush987goyal
Copy link
Contributor Author

@ayush987goyal ayush987goyal commented Feb 19, 2021

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.

error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-apigatewayv2.IHttpApi] construct interface must extend core.IConstruct 
error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-apigatewayv2.IWebSocketApi] construct interface must extend core.IConstruct 

error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-apigatewayv2.IHttpApi] construct interfaces of AWS resources must extend cdk.IResource
error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-apigatewayv2.IWebSocketApi] construct interfaces of AWS resources must extend cdk.IResource
Copy link
Contributor

@nija-at nija-at left a 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.

packages/@aws-cdk/aws-apigatewayv2/lib/common/api.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/common/api.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/common/api.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/common/api.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/lib/http/stage.ts Outdated Show resolved Hide resolved
@ayush987goyal ayush987goyal force-pushed the ayush987goyal:pr/ws branch from a89e3e5 to a066643 Feb 27, 2021
@mergify mergify bot dismissed nija-at’s stale review Feb 27, 2021

Pull request has been modified.

@ayush987goyal ayush987goyal requested a review from nija-at Feb 27, 2021
Copy link
Contributor

@nija-at nija-at left a comment

This is fantastic!
Just 1 or 2 comments around the design/refactor, the rest are just small improvements and clarifications.

@ayush987goyal ayush987goyal requested a review from nija-at Mar 3, 2021
@mergify mergify bot dismissed nija-at’s stale review Mar 3, 2021

Pull request has been modified.

This was referenced Mar 8, 2021
cornerwings added a commit to cornerwings/aws-cdk that referenced this pull request Mar 8, 2021
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants