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

Add baseDir and resolve build options to serverless-trace #779

Merged
merged 7 commits into from
Nov 11, 2020

Conversation

ramosbugs
Copy link
Contributor

This PR makes two changes to the lambda-at-edge builder to better
support Yarn v2 projects when using the serverless-trace target.

  1. Adds a baseDir build option to specify the base directory to search
    for node_modules. Currently, the builder sets this to process.cwd(),
    but Yarn v2 often hoists dependencies across multiple workspaces so
    that they can be shared. Without this change, all dependencies from
    ancestor directories are omitted, leading to import errors at runtime.
  2. Adds a resolve build option which allows projects to specify their
    own custom resolvers, such as when supporting Yarn v2's PnP mode.
    This leverages the resolver hook added in Expose hook into the module resolution (resolve) vercel/nft#153, which also
    required upgrading from @zeit/node-file-trace to the latest version
    of @vercel/nft. Note that even after this change, Yarn v2's PnP mode
    is still incompatible with the builder since the builder ends up
    collapsing all of the PnP dependencies into a single node_modules
    directory. This causes multiple versions of a package to clobber one
    another.

cc: @sergioramos (no relation 😆)

This PR makes two changes to the `lambda-at-edge` builder to better
support Yarn v2 projects when using the `serverless-trace` target.

1. Adds a `baseDir` build option to specify the base directory to search
   for `node_modules`. Currently, the builder sets this to `process.cwd()`,
   but Yarn v2 often hoists dependencies across multiple workspaces so
   that they can be shared. Without this change, all dependencies from
   ancestor directories are omitted, leading to import errors at runtime.
2. Adds a `resolve` build option which allows projects to specify their
   own custom resolvers, such as when supporting Yarn v2's PnP mode.
   This leverages the resolver hook added in vercel/nft#153, which also
   required upgrading from `@zeit/node-file-trace` to the latest version
   of `@vercel/nft`. Note that even after this change, Yarn v2's PnP mode
   is still incompatible with the builder since the builder ends up
   collapsing all of the PnP dependencies into a single `node_modules`
   directory. This causes multiple versions of a package to clobber one
   another.
@ramosbugs
Copy link
Contributor Author

I'm not able to repro the CI failures locally... both yarn test and yarn integration are passing :-/

@danielcondemarin
Copy link
Contributor

I'm not able to repro the CI failures locally... both yarn test and yarn integration are passing :-/

Thanks for PR'ing! I'll take a look this weekend at why CI is failing 👍

@dphang
Copy link
Collaborator

dphang commented Nov 7, 2020

Yea, I was trying to upgrade this as well, but got the same failures. Not quite sure what is happening here myself.

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #779 (c2e2243) into master (3ff7563) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #779      +/-   ##
==========================================
+ Coverage   79.97%   80.00%   +0.02%     
==========================================
  Files          56       56              
  Lines        1833     1835       +2     
  Branches      404      406       +2     
==========================================
+ Hits         1466     1468       +2     
  Misses        309      309              
  Partials       58       58              
Impacted Files Coverage Δ
packages/libs/lambda-at-edge/src/build.ts 93.42% <100.00%> (+0.05%) ⬆️

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 3ff7563...c2e2243. Read the comment docs.

@danielcondemarin
Copy link
Contributor

danielcondemarin commented Nov 11, 2020

@ramosbugs I noticed you didn't add the baseDir and resolve option to the component's inputs. Out of interest are you using lambda-at-edge as a standalone lib for your project?

@ramosbugs
Copy link
Contributor Author

ramosbugs commented Nov 11, 2020

thanks for getting to the bottom of the CI issue!

@ramosbugs I noticed you didn't add the baseDir and resolve option to the component's inputs. Out of interest are you using lambda-at-edge as a standalone lib for your project?

yup, I'm invoking the builder from a custom build script, then deploying it with CloudFormation AWS SAM.

@danielcondemarin
Copy link
Contributor

thanks for getting to the bottom of the CI issue!

@ramosbugs I noticed you didn't add the baseDir and resolve option to the component's inputs. Out of interest are you using lambda-at-edge as a standalone lib for your project?

yup, I'm invoking the builder from a custom build script, then deploying it with CloudFormation AWS SAM.

That's really interesting! We're thinking of moving serverless-next.js over to AWS CDK or CDK for Terraform for first party deployments. SAM is another interesting choice, would be good to hear if you have any thoughts on this :)

Btw just going to run the E2E tests locally now and get this PR merged.

Copy link
Contributor

@danielcondemarin danielcondemarin left a comment

Choose a reason for hiding this comment

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

E2E looks good. Had to add a new test app for useServerlessTrace: true but will create a separate PR for that.

@danielcondemarin danielcondemarin merged commit 8f978bb into serverless-nextjs:master Nov 11, 2020
@ramosbugs
Copy link
Contributor Author

That's really interesting! We're thinking of moving serverless-next.js over to AWS CDK or CDK for Terraform for first party deployments. SAM is another interesting choice, would be good to hear if you have any thoughts on this :)

I don't have any experience with CDK or Terraform, so I can't provide a direct comparison, unfortunately. This is also my first substantial Lambda-based project. SAM has been a mixed experience. It's only been "generally available" for ~4 months, and it's still a bit immature with some rough edges, but I think they're actively improving it as they get feedback.

Most of my troubles have been around the backend (AWS API Gateway-based) API deployment. For example, local API testing is quite slow since SAM spins up a new Docker container on every API request. My other issues are with missing functionality going from an OpenAPI spec to a set of backend Lambdas, which I've solved by adding some Jinja2-based templating on top of my SAM/CloudFormation templates.

The Next.JS deployment itself was pretty easy, though:

Resources:
  NextJsSsr:
    Type: AWS::Serverless::Function
    Properties:
      AutoPublishAlias: live
      CodeUri: ../../webapp
      Description: Lambda@Edge handler for Next.JS server-side rendering
      VersionDescription: {{ git_hash }} # Filled in by Jinja2
      FunctionName: !Sub "${EnvName}-frontend-next-js-ssr"
      Handler: index.handler
      MemorySize: 3008
      Role: !GetAtt NextJsSsrExecRole.Arn
      Runtime: nodejs12.x
      Timeout: 10
    Metadata:
      BuildMethod: makefile

  NextJsSsrExecRole:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Version: 2012-10-17
        Statement:
          - Action:
              - sts:AssumeRole
            Effect: Allow
            Principal:
              Service:
                - edgelambda.amazonaws.com
                - lambda.amazonaws.com
      ManagedPolicyArns:
        - arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole
      RoleName: !Sub "${EnvName}-frontend-next-js-ssr-exec-role"

Makefile (avoids unnecessary Next.JS builds on every sam build invocation):

.next/prerender-manifest.json: $(shell find assets components fonts lib pages public styles -type f) .babelrc build.js next.config.js package.json tsconfig.json
	yarn install && yarn build

build-NextJsSsr: .next/prerender-manifest.json
	cp -pR dist/default-lambda/* $(ARTIFACTS_DIR)

Btw just going to run the E2E tests locally now and get this PR merged.

thank you!

@ramosbugs
Copy link
Contributor Author

err, actually, the above is just the Lambda@Edge deployment. there's a whole other SAM template for pushing the static Next.JS files to S3, which I ended up having to write a custom CloudFormation resource for. It uses a Python-based Lambda to upload static assets to S3, and also duplicates a lot of the serverless component's functionality around setting CloudFront cache behaviors, etc.

I do love that the output is declarative though 🤷

@danielcondemarin
Copy link
Contributor

I don't have any experience with CDK or Terraform, so I can't provide a direct comparison, unfortunately. This is also my first substantial Lambda-based project. SAM has been a mixed experience. It's only been "generally available" for ~4 months, and it's still a bit immature with some rough edges, but I think they're actively improving it as they get feedback.

Thanks for sharing. That's quite similar to what I've heard from colleagues that have used it.

I think we need something a bit more mature and stable. CDK has been out for some time now and it has 2 big advantages for us, one is that Terraform now supports it which could be beneficial in future when we support other cloud providers. The other is that CDK's architecture is quite modular, and allows you to plug in your own constructs with relative ease, this makes it easy for us to build on top of it, i.e. a serverless-next.js construct.

@danielcondemarin
Copy link
Contributor

@ramosbugs This is now available in @sls-next/[email protected] 👍

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

Successfully merging this pull request may close these issues.

3 participants