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

Discuss removing serve command #135

Closed
hassankhan opened this issue Jun 30, 2017 · 29 comments
Closed

Discuss removing serve command #135

hassankhan opened this issue Jun 30, 2017 · 29 comments

Comments

@hassankhan
Copy link
Contributor

hassankhan commented Jun 30, 2017

Given that serverless-offline and serverless-simulate both exist, are there many users of this plugin that use the included serve functionality?

Removing it would help a lot with code bloat, allowing the plugin to focus solely on building bundles via Webpack.

Any feedback from users would highly be appreciated, would be great to know what plugin/combinations of plugins you use for development.

@HyperBrain
Copy link
Member

Thanks @hassankhan for bringing this up 👍 . Maintaining code and functionality that is probably not used or completely replaced by other plugins is wasted effort.
Regarding the versioning schematics, if decided to be removed, this must go into a 2.0.0 version.

@franciscocpg
Copy link
Member

I agree with @hassankhan. Another plugins are already focusing on simulate an API gateway and they integrate well with serverless-webpack.

@hassankhan
Copy link
Contributor Author

hassankhan commented Jun 30, 2017

Agreed @HyperBrain, if it is indeed removed, it should definitely be a major version bump.

@HyperBrain
Copy link
Member

HyperBrain commented Jun 30, 2017

I linked all open PR's that are related to the serve functionality here. The PR authors might check back here and tell us the status of their serve usage then.
These PRs should not be selected as candidates for the next release until this question has not been answered.

@Whoaa512
Copy link

Ok so I do use the serve functionality but only b/c I can't get serverless-offline to actually use my webpack config.

If I can get that figured out I'm totally fine removing the serve functionality.

@franciscocpg
Copy link
Member

@Whoaa512
I'm using serverless-webpack with serverless-offline.
Which problems are you facing?

@Whoaa512
Copy link

So I'm using [email protected] and running sls --stage dev offline start

In my webpack.entry config I have

    entry: {
        './handlers/lambda': ['../baseConfig.js', './handlers/lambda.js'],
    },

But for some reason my baseConfig doesn't get included in the bundle when I run serverless-offline

Not sure if I'm missing something else.

@Whoaa512
Copy link

Whoaa512 commented Jun 30, 2017

Turns out I just needed to use sls --stage dev offline removing the start on makes it work as expected.

So I'm fine with removing the serve functionality.

@arabold
Copy link

arabold commented Jun 30, 2017

We're successfully using serverless-offline in combination with serverless-webpack since months. I tried theserve command once but it didn't really work well and didn't provide the full simulation as serverless-offline does. There are A LOT of things to consider when trying to simulate API Gateway correctly.

So I'm absolutely for removing the serve command from serverless-webpack and redirect users to serverless-offline instead. This will allow focusing on the core features which are packaging, not simulating IMO.

@franciscocpg
Copy link
Member

Did you follow this https://github.com/dherault/serverless-offline#usage-with-serverless-offline-and-serverless-webpack-plugin?
Take note that according to docs, serverless-offline must be the last item in the plugin configuration.

@franciscocpg
Copy link
Member

franciscocpg commented Jun 30, 2017

Ops I hadn't read #135 (comment)

@HyperBrain
Copy link
Member

I'll tag all PRs relevant to the serve functionality with the pr/serve tag, so that they are visible. If we take the decision to remove serve we can close the tagged ones for the specific release.

@HyperBrain
Copy link
Member

According to all the feedback that we received either here or within the open PRs, it seems feasible that we remove the serve functionality. Serverless-offline can be used for anything that also could be done with serve.

@hassankhan
Copy link
Contributor Author

Big +1, glad to hear that

@dillonbailey
Copy link
Contributor

dillonbailey commented Jul 8, 2017

I have been running into an issue when using lambda proxy.

Testing locally with sls webpack serve, event.body object can be accessed directly.

Running the function on AWS I need to run JSON.parse(event.body) in order to access the object. Attempting to run JSON.parse() locally results in an error because the object is not stringified.

Would this be related to all of the chatter above of simulation errors?

Cheers!

@HyperBrain
Copy link
Member

HyperBrain commented Jul 8, 2017

@dillonbailey The event.body property of Lambda proxy integrations normally are received as string (i.e. you have to parse the body after checking the ContentType header).
If serverless webpack serve gives you an object instead, this is wrong. As you see in this task, we're planning to remove the serve command.

Could you try to use the serverless-offline plugin instead to simulate a local API Gateway? It offers the same functionality and works together with the webpack plugin. It is important that you add the offline plugin AFTER the webpack plugin in your serverless.yml.

@arabold
Copy link

arabold commented Jul 8, 2017

There's a difference between lambda and lambda-proxy integration in Serverless. I don't remember which one works which way, but over the last two years a pattern has emerged for us:

const body = _.isString(event.body) ? JSON.parse(event.body) : event.body;

This seems to be the most reliable and robust way around different behaviors of the simulation. 🤔

@dillonbailey
Copy link
Contributor

@HyperBrain thank heaps! Will give the serverless-offline plugin a try and let you know how it goes!

@arabold indeed a great little pattern to use! I think there is also a difference with lambda vs. lambda-proxy where one passes the events directly to the function e.g. event.variable_name and the other provides them via event.body

@hassankhan
Copy link
Contributor Author

Should we open a new issue for discussing what to do about the existing PRs for the serve functionality? The purpose of this issue was to decide whether or not to remove it, now that we've decided we should probably close this 😄

@HyperBrain
Copy link
Member

HyperBrain commented Jul 12, 2017

I'm just about to create an issue to actually remove the serve functionality ;-)

Regarding the PRs and issues related to serve, my opinion is to close all of them with a comment that references the task. Spending efforts on a feature that is about to be dropped doesn't make much sense.

@HyperBrain
Copy link
Member

Closing this now - the removal of serve and the technical discussion is done in #152

@bebbi
Copy link

bebbi commented Aug 8, 2017

Per @HyperBrain request, reposting question here:

I've tried to run

serverless offline start

instead of

serverless webpack serve

In the offline way, the lambdas get called, execute correctly - but they don't return a json body as they do with the serve way. I don't see any related issue in serverless-offline, maybe something related to serverless-webpack?

I'm happy to troubleshoot but don't know where to start.

@franciscocpg To answer your question, no it's in the format

callback(null, { status: 'success', data: 'whatever' }

because I'm using the Lambda integration, not the Lambda Proxy Integration.

The relevant pieces of my serverless.yml look like this:

integration: lambda
request:
  passThrough: NEVER
response:
  headers:
    Content-Type: "'application/json'"

@bebbi
Copy link

bebbi commented Aug 8, 2017

I wonder if the response fails as I'm not defining a body mapping template. A template is not required though (I'm currently searching for the AWS spec I found that in a long time ago) and in the real AWS integration, a default template is used if missing.

Some logs from my offline run that are perhaps of interest

[offline] Using response 'default'
[offline] _____ RESPONSE PARAMETERS PROCCESSING _____
[offline] Found 0 responseParameters for 'default' response
[offline] _____ RESPONSE TEMPLATE PROCCESSING _____
[offline] Using responseTemplate 'application/json'
[offline] Processing key: root - value: 

[offline] Velocity rendered: 

Serverless: [200] "\n"

@bebbi
Copy link

bebbi commented Aug 8, 2017

for lack of finding docs, here's a screenshot of the "integration response" section in aws when uploading with above serverless.yml config - note the "Default mapping" there.

image

@franciscocpg
Copy link
Member

@bebbi
I have no experience with Integration: Lambda but it's working fine from here.
Are you calling the callback this way?

@bebbi
Copy link

bebbi commented Aug 10, 2017

@franciscocpg thanks good pointer, will check against there.

@HyperBrain Just as a heads up, there may be more blockers for me (others?) to switch from serve to serverless. I just submitted a fix for one in dherault/serverless-offline#282

I'll try to figure out the above one.

@HyperBrain
Copy link
Member

@bebbi Yes, that's true. Nevertheless the plugin should concentrate on the packaging. If we encounter issues, we should try to push the changes/fixes into serverless-offline. I think @dherault would also agree that the combination of both plugins create a system that is superior to having similar functionality maintained in both plugins.

And thanks for your engagement 🙌

@bebbi
Copy link

bebbi commented Aug 10, 2017

@HyperBrain agree on approach, it's just about the transition.
See dherault/serverless-offline#284 for issue and a fixing PR -
maybe 3.0 can be contingent on the two PRs being integrated and published?
@franciscocpg I was curious why it worked for the test you linked - because of a bug :)

@HyperBrain
Copy link
Member

@bebbi Agree. I try to reach out to @dherault to get this in asap. In the meantime I'll release further v3 fixes as rcs but merge it to master (there will be no dev on v2 anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants