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.

Sign up for GitHub

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

Jump to bottom

grouped all websockets-related properties under provider.websocket na… #10963

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

grouped all websockets-related properties under provider.websocket na… #10963

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e784f04
grouped all websockets-related properties under provider.websocket na…
BillConley01 Apr 12, 2022
7578f22
checks for websocket object
BillConley01 Apr 14, 2022
1dd2ec3
default values for instances websockets properties are not defined.
BillConley01 Apr 30, 2022
9183f30
fix typo
BillConley01 Apr 30, 2022
6f85676
typo fix
BillConley01 Apr 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/providers/aws/events/websocket.md
View file
Open in desktop
Expand Up @@ -76,9 +76,10 @@ service: serverless-ws-test
provider:
name: aws
runtime: nodejs12.x
websocketsApiName: custom-websockets-api-name
websocketsApiRouteSelectionExpression: $request.body.action # custom routes are selected by the value of the action property in the body
websocketsDescription: Custom Serverless Websockets
websockets:
apiName: custom-websockets-api-name
apiRouteSelectionExpression: $request.body.action # custom routes are selected by the value of the action property in the body
description: Custom Serverless Websockets

functions:
connectionHandler:
Expand Down
11 changes: 6 additions & 5 deletions docs/providers/aws/guide/serverless.yml.md
View file
Open in desktop
Expand Up @@ -238,11 +238,12 @@ provider:
# Endpoint type for API Gateway REST API: edge or regional (default: edge)
endpointType: regional
# Use a custom name for the websockets API
websocketsApiName: custom-websockets-api-name
# custom route selection expression
websocketsApiRouteSelectionExpression: $request.body.route
# Use a custom description for the websockets API
websocketsDescription: Custom Serverless Websockets
websockets:
apiName: custom-websockets-api-name
# custom route selection expression
apiRouteSelectionExpression: $request.body.route
# Use a custom description for the websockets API
description: Custom Serverless Websockets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move websockets below apiGateway? (I think apiGateway is used by more people, we should make it a bit more visible)

Also there's the websocketApiId property that isn't moved to websockets, is that intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matthieu, I will move Websockets after APIGateway. I didn't group websocketApiID with Websockets because it was already a property of APIGateway?

# Optional API Gateway REST API global config
apiGateway:
# Attach to an externally created REST API via its ID:
Expand Down
3 changes: 3 additions & 0 deletions lib/plugins/aws/lib/naming.js
View file
Open in desktop
Expand Up @@ -207,6 +207,9 @@ module.exports = {

// Websockets API
getWebsocketsApiName() {
if (_.get(this.provider.serverless.service.provider.websockets)) {
return `${this.provider.serverless.service.provider.websockets.apiName}`;
}
if (
this.provider.serverless.service.provider.websocketsApiName &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still support the old syntax as well, but first check for the new one

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only one I can't get to pass the tests. I used the code below. Any suggestions that might point me in the right direction?
getWebsocketsApiName() {
if (this.provider.serverless.service.provider.websockets.apiName &&
typeof this.provider.serverless.service.provider.websockets.apiName === 'string') {
return ${this.provider.serverless.service.provider.websockets.apiName};
}
if(this.provider.serverless.service.provider.websocketsApiName &&
typeof this.provider.serverless.service.provider.websocketsApiName === 'string')
{
return ${this.provider.serverless.service.provider.websocketsApiName};
}
return ${this.provider.getStage()}-${this.provider.serverless.service.service}-websockets;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also follow a similar approach with using _.get from lodash. We should not check the type of the property but rather only the fact that it exists, the schema validation checks that it's a string if its defined.

Small request - when pasting the code samples, please format them all with code blocks as it's hard to read unformatted code. Thanks in advance 🙇

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the test are passing now. Should I just push my changes to the branch or push and create a new pull request?

Copy link
Contributor

@pgrzesik pgrzesik Apr 15, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests might be passing, but just glancing over the code I see that a lot of stuff listed in the issue is not addressed, I'm also pretty sure it's going to break in a few scenarios. Are you sure the PR in current form is ready for a deeper review?

I can also review it in the current state and provide feedback - please let me know 🙌

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your feedback. This is a learning process for me and I have identified several things I could have done better process wise and learned about lodash in the process. I'd like to address all the requirements for this issue. I see that I missed the deprecation.md. I assume that serverless._logDeprecation is a terminal command. I'll let you know when i resolve this and the other issues you mentioned.
Could you give me a hint of the break scenarios? Are they related to the lodash get?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @BillConley01 - no worries, this issue is actually a bit more complex that it might've seem at first - I'll try to provide more details to help you understand the scope better.

For the potential breaking scenario, let's consider this check

getWebsocketsApiName() {
    if (_.get(this.provider.serverless.service.provider.websockets)) {
      return `${this.provider.serverless.service.provider.websockets.apiName}`;
    ....
}

In the check above, we're checking if the this.provider.serverless.service.provider.websockets is set, but that doesn't mean that this.provider.serverless.service.provider.websockets.apiName is set as well - we might have a situation where someone defined some other properties under provider.websockets, but didn't define apiName there - in such situation we should fallback to default value instead of returning an invalid string here.

Another example we can find here:

Description: this.serverless.service.provider.websockets
            ? this.serverless.service.provider.websockets.description
            : this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',

This is a part of the code that resolves the description. In the ternary here, we are checking if provider.websockets is defined - if it is, we return provider.websocket.description, but what if the provider.websockets.description is not defined? In that case, we would set it to undefined, where we would want to have the default of Serverless Websockets instead.

Hope that helps a bit - if something is not clear, sorry about that and please let me know, I'll try to explain it a bit better 🙇

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation and I apologize for the delay. A family member passed away last week. I addressed the issues mentioned and updated the readme files. The only thing I am unsure of is where/how to add the deprecation code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @BillConley01 - no worries about the delay and really sorry about your loss.

As for the deprecation code, you can see how that can be implemented e.g. in this PR - #8277

If you have any additional questions, feel free to reach out 💯

typeof this.provider.serverless.service.provider.websocketsApiName === 'string'
Expand Down
16 changes: 11 additions & 5 deletions lib/plugins/aws/package/compile/events/websockets/lib/api.js
View file
Open in desktop
Expand Up @@ -13,18 +13,24 @@ module.exports = {

this.websocketsApiLogicalId = this.provider.naming.getWebsocketsApiLogicalId();

const RouteSelectionExpression =
this.serverless.service.provider.websocketsApiRouteSelectionExpression ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still support the old syntax, but checking the new one first when resolving the property

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep getting more test failures. I think I am in above my head on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is currently causing you troubles? From what I see in the errors - it's failing because it tries to access websocket.apiName when websocket is not defined at all - we have to keep that in mind that people might not define anything under provider.websocket so it can be undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would something like this resolve const RouteSelectionExpression?
const RouteSelectionExpression = this.serverless.service.provider.websockets.apiRouteSelectionExpression ? this.serverless.service.provider.websockets.apiRouteSelectionExpression || '$request.body.action': this.serverless.service.provider.websocketsApiRouteSelectionExpression || '$request.body.action';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way would be to use _.get() from lodash - you can see a few examples on how to use it in naming.js module

'$request.body.action';
const RouteSelectionExpression = _.get(
this.provider,
'serverless.service.provider.websockets.routeSelectionExpression'
)
? this.serverless.service.provider.websockets.routeSelectionExpression ||
'$request.body.action'
: this.serverless.service.provider.websocketsRouteSelectionExpression ||
'$request.body.action';

_.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {
[this.websocketsApiLogicalId]: {
Type: 'AWS::ApiGatewayV2::Api',
Properties: {
Name: this.provider.naming.getWebsocketsApiName(),
RouteSelectionExpression,
Description:
this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still support the old syntax, but checking the new one first when resolving the property

Description: this.serverless.service.provider.websockets
? this.serverless.service.provider.websockets.description
: this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
ProtocolType: 'WEBSOCKET',
},
},
Expand Down
5 changes: 3 additions & 2 deletions lib/plugins/aws/package/compile/events/websockets/lib/deployment.js
View file
Open in desktop
Expand Up @@ -34,8 +34,9 @@ module.exports = {
DependsOn: dependentResourceIds,
Properties: {
ApiId: this.provider.getApiGatewayWebsocketApiId(),
Description:
this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
Description: this.serverless.service.provider.websockets
? this.serverless.service.provider.websockets.description
: this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
},
},
});
Expand Down
5 changes: 3 additions & 2 deletions lib/plugins/aws/package/compile/events/websockets/lib/stage.js
View file
Open in desktop
Expand Up @@ -30,8 +30,9 @@ module.exports = {
ApiId: this.provider.getApiGatewayWebsocketApiId(),
// DeploymentId is generated at deployment.js file
StageName: this.provider.getStage(),
Description:
this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still support the old syntax, but checking the new one first when resolving the property

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will add the old back to those four areas. Thanks.

Description: this.serverless.service.provider.websockets
? this.serverless.service.provider.websockets.description
: this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',
},
};

Expand Down
9 changes: 9 additions & 0 deletions lib/plugins/aws/provider.js
View file
Open in desktop
Expand Up @@ -1198,6 +1198,15 @@ class AwsProvider {
websocketsApiName: { type: 'string' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still support the old properties - removing them is a breaking change and we want to avoid that until releasing new major in the future - hence the need for deprecation first

websocketsApiRouteSelectionExpression: { type: 'string' },
websocketsDescription: { type: 'string' },
websockets: {
type: 'object',
properties: {
apiName: { type: 'string' },
apiRouteSelectionExpression: { type: 'string' },
description: { type: 'string' },
},
additionalProperties: false,
},
},
},
function: {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/lib/plugins/aws/lib/naming.test.js
View file
Open in desktop
Expand Up @@ -233,7 +233,7 @@ describe('#naming()', () => {
});
});

describe('#getWebsocketsApiName()', () => {
describe('#get()', () => {
it('should return the composition of stage & service name if custom name not provided', () => {
serverless.service.service = 'myService';
expect(sdk.naming.getWebsocketsApiName()).to.equal(
Expand Down