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

BillConley01 · 2022-04-12T16:41:03Z

…namespace
Made required changes but websocket index tests fail. Can you please take a look.

Addresses: #{10960}

…mespace

pgrzesik

Thanks @BillConley01 - that's a good start but in current form it would become a breaking change - we need to still support old properties while at the same time promoting the new ones (and issuing deprecation notice when someone uses the old ones).

pgrzesik · 2022-04-12T19:32:55Z

lib/plugins/aws/lib/naming.js

@@ -208,10 +208,10 @@ module.exports = {
  // Websockets API
  getWebsocketsApiName() {
    if (
-      this.provider.serverless.service.provider.websocketsApiName &&


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

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;
}

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 🙇

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

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 🙌

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?

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 🙇

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?

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 💯

pgrzesik · 2022-04-12T19:33:21Z

lib/plugins/aws/package/compile/events/websockets/lib/api.js

@@ -14,7 +14,7 @@ module.exports = {
    this.websocketsApiLogicalId = this.provider.naming.getWebsocketsApiLogicalId();

    const RouteSelectionExpression =
-      this.serverless.service.provider.websocketsApiRouteSelectionExpression ||


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

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

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

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';

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

pgrzesik · 2022-04-12T19:33:26Z

lib/plugins/aws/package/compile/events/websockets/lib/api.js

@@ -24,7 +24,7 @@ module.exports = {
          Name: this.provider.naming.getWebsocketsApiName(),
          RouteSelectionExpression,
          Description:
-            this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',


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

pgrzesik · 2022-04-12T19:33:36Z

lib/plugins/aws/package/compile/events/websockets/lib/stage.js

@@ -31,7 +31,7 @@ module.exports = {
          // DeploymentId is generated at deployment.js file
          StageName: this.provider.getStage(),
          Description:
-            this.serverless.service.provider.websocketsDescription || 'Serverless Websockets',


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

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

pgrzesik · 2022-04-12T19:34:07Z

lib/plugins/aws/provider.js

@@ -1195,9 +1195,15 @@ class AwsProvider {
            vpc: { $ref: '#/definitions/awsLambdaVpcConfig' },
            vpcEndpointIds: { $ref: '#/definitions/awsCfArrayInstruction' },
            versionFunctions: { $ref: '#/definitions/awsLambdaVersioning' },
-            websocketsApiName: { type: 'string' },


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

mnapoli · 2022-04-22T11:39:46Z

docs/providers/aws/guide/serverless.yml.md

+    # custom route selection expression
+    apiRouteSelectionExpression: $request.body.route
+    # Use a custom description for the websockets API
+    description: Custom Serverless Websockets


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?

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

codecov · 2022-04-22T11:46:30Z

Codecov Report

Merging #10963 (7c0d424) into main (89a92a3) will decrease coverage by 0.46%.
The diff coverage is 65.62%.

❗ Current head 7c0d424 differs from pull request most recent head 6f85676. Consider uploading reports for the commit 6f85676 to get more accurate results

@@            Coverage Diff             @@
##             main   #10963      +/-   ##
==========================================
- Coverage   86.38%   85.91%   -0.47%     
==========================================
  Files         305      311       +6     
  Lines       12947    13148     +201     
==========================================
+ Hits        11184    11296     +112     
- Misses       1763     1852      +89

Impacted Files	Coverage Δ
bin/serverless.js	`47.05% <0.00%> (-4.01%)`	⬇️
commands/plugin-uninstall.js	`65.07% <ø> (ø)`
lib/cli/run-compose.js	`0.00% <0.00%> (ø)`
lib/config-schema.js	`100.00% <ø> (ø)`
lib/configuration/variables/index.js	`90.47% <ø> (ø)`
lib/plugins/aws/deploy/lib/check-for-changes.js	`96.85% <ø> (ø)`
lib/plugins/aws/package/compile/events/s3/index.js	`98.54% <ø> (ø)`
...ackage/compile/events/websockets/lib/deployment.js	`100.00% <ø> (+4.76%)`	⬆️
lib/plugins/aws/provider.js	`94.59% <ø> (ø)`
lib/plugins/aws/lib/naming.js	`96.93% <50.00%> (-0.49%)`	⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a92a3...6f85676. Read the comment docs.

default values for instances websockets properties are not defined.

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

e784f04

…mespace

pgrzesik suggested changes Apr 12, 2022

View reviewed changes

checks for websocket object

7578f22

mnapoli reviewed Apr 22, 2022

View reviewed changes

BillConley01 added 3 commits April 30, 2022 14:29

default values for instances websockets properties are not defined.

1dd2ec3

default values for instances websockets properties are not defined.

fix typo

9183f30

typo fix

6f85676

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BillConley01 commented Apr 12, 2022

pgrzesik left a comment

pgrzesik Apr 12, 2022

BillConley01 Apr 13, 2022

pgrzesik Apr 14, 2022

BillConley01 Apr 14, 2022 •

edited

pgrzesik Apr 15, 2022 •

edited

BillConley01 Apr 15, 2022

pgrzesik Apr 19, 2022

BillConley01 Apr 30, 2022

pgrzesik May 8, 2022

pgrzesik Apr 12, 2022

BillConley01 Apr 12, 2022

pgrzesik Apr 13, 2022

BillConley01 Apr 13, 2022

pgrzesik Apr 13, 2022

pgrzesik Apr 12, 2022

pgrzesik Apr 12, 2022

BillConley01 Apr 12, 2022

pgrzesik Apr 12, 2022

mnapoli Apr 22, 2022

BillConley01 Apr 29, 2022

codecov bot commented Apr 22, 2022 •

edited

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

Are you sure you want to change the base?

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

Conversation

BillConley01 commented Apr 12, 2022

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillConley01 Apr 14, 2022 • edited

Choose a reason for hiding this comment

pgrzesik Apr 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2022 • edited

Codecov Report

BillConley01 Apr 14, 2022 •

edited

pgrzesik Apr 15, 2022 •

edited

codecov bot commented Apr 22, 2022 •

edited