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

Fix: Non node functions are attempted to be packaged #808

Conversation

roni-frantchi
Copy link

@roni-frantchi roni-frantchi commented May 3, 2021

What did you implement:

Closes --

#579 helped in making webpack not compile any non-node functions, BUT the plugin still attempts to package what it think would be webpack's output and fails miserably.

This PR will make it so that non node runtime packages are also not attempted to be packaged.

How did you implement it:

packageModules.js will now only package functions set as entryFunctions so that any functions that were not not listed as entry points will not attempted to be packaged.

How can we verify it:

There's a unit test added, plus you could:

provider:
  name: aws
  runtime: nodejs12.x 

functions:
  node:
    handler: lambda.echo

  py:
    handler: backend.lambda.LambdaFunction
    runtime: python 

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR (can't check this as this is an organization PR)
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

Comment on lines +143 to +145
const func = this.serverless.service.getFunction(funcName);
const runtime = func.runtime || this.serverless.service.provider.runtime || 'nodejs';
return runtime.match(/node/);
Copy link
Member

Choose a reason for hiding this comment

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

Can you create a separate function to re-use this code from line 217?
For example, you can create that function just below the getHandlerFile function:

    const isNodeRuntime = funcName => {
      const func = this.serverless.service.getFunction(funcName);
      const runtime = func.runtime || this.serverless.service.provider.runtime || 'nodejs';

      return runtime.match(/node/);
    };

And then, re-use it here (and when we build the allEntryFunctions const):

Suggested change
const func = this.serverless.service.getFunction(funcName);
const runtime = func.runtime || this.serverless.service.provider.runtime || 'nodejs';
return runtime.match(/node/);
return isNodeRuntime(funcName);

Choose a reason for hiding this comment

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

Another option might be to add something like a getAllNodeFunctions() at a higher to replace this.serverless.service.getAllFunctions() calls with a filtered version. This could then also be used to provide similar logic in files like validate.js.

@@ -139,7 +139,12 @@ module.exports = {

// Copy artifacts to package location
if (isIndividialPackaging.call(this)) {
_.forEach(functionNames, funcName => copyArtifactByName.call(this, funcName));
const nodeFunctionNames = _.filter(functionNames, funcName => {
Copy link

@timbodeit timbodeit May 6, 2021

Choose a reason for hiding this comment

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

The same nodeFunctionNames would also need to be used below in lines 149 (previously 154) and 165 (previously 160) to avoid serverless-webpack still overriding the artifact path of the given function.

@murilolobato
Copy link

murilolobato commented May 12, 2021

There is any workaround for webpack avoid packaging a python function while this solution is not merged into master?

@j0k3r
Copy link
Member

j0k3r commented Jun 10, 2021

Done by #876

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reply Awaiting for a reply from the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants