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

feat: add edge support to aws-lambda presets #1557

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

Conversation

Hebilicious
Copy link
Contributor

@Hebilicious Hebilicious commented Aug 9, 2023

πŸ”— Linked issue

Resolves #79
Related #240
Related #1075

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Continuing on the amazing work done in #240 and #1075, this PR implements lambda edge as an option of the lambda-preset, usable with { target: edge } adds a CDK construct based on @WinterYukky work, and adds a SST construct.

If we want to provide something that is directly deployable (ie for nitro-deploys), the CDK is an interesting option, however looking closely at it, the CDK itself has a few drawbacks :

  • It is very verbose and requires 2 separated stacks for lambda and lambda-edge
  • It adds a large amount of maintenance burden and AWS CDK overhead
  • It is quite a low level tool and not straightforward to use
  • We heavily get into the SST / Pulumi / SLS territory

Supporting CDK options is not straight forward... We could provide a Lambda-single and a Lambda-edge stack, and mirror some of these options in the config under a cdkOptions key. This is quite limited and I would assume most users will replace this with their own stack in real world scenario.

I think the best course on action here would be to rely on the higher level constructs provided by SST, example here
https://github.com/jdevdevdev/nuxt-sst/blob/lambda-edge/stacks/NuxtSite.ts

This PR therefore adds a SST implementation that extends the SSRSite constructs to compare with the CDK ones.
(Later we could contribute it upstream to simplify the preset)

For reference, there is also the possibility of using Amplify with the CDK.
As usual with AWS, this is poorly documented, and we should deal with this in another PR #80

(PS: I would like to give credit to @WinterYukky, @juankamilo and @jdevdevdev as co-authors of this PR)

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1557 (baf8d9c) into main (fffd872) will decrease coverage by 3.71%.
The diff coverage is 10.95%.

❗ Current head baf8d9c differs from pull request most recent head 8c4bf2c. Consider uploading reports for the commit 8c4bf2c to get more accurate results

@@            Coverage Diff             @@
##             main    #1557      +/-   ##
==========================================
- Coverage   77.76%   74.05%   -3.71%     
==========================================
  Files          76       74       -2     
  Lines        7824     7871      +47     
  Branches      803      756      -47     
==========================================
- Hits         6084     5829     -255     
- Misses       1738     2041     +303     
+ Partials        2        1       -1     
Files Changed Coverage Ξ”
src/utils/cdk.ts 3.00% <3.00%> (ΓΈ)
src/presets/aws-lambda.ts 93.33% <92.30%> (-6.67%) ⬇️

... and 20 files with indirect coverage changes

Copy link
Contributor Author

@Hebilicious Hebilicious left a comment

Choose a reason for hiding this comment

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

Both the CDK and SST logic can be removed from this PR and included later. However for nitro-deploys we need one or the other.

@@ -359,6 +359,7 @@ async function _build(nitro: Nitro, rollupConfig: RollupConfig) {
const buildInfo = {
date: new Date(),
preset: nitro.options.preset,
entry: nitro.options.entry.split("/").pop(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have no way to know which entry a preset use, maybe this should be in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted in #1649

}) =>
/* ts */ `
import { SSTConfig } from "sst";
import { NitroApp } from "${sstDir}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be a relative path.

@Hebilicious Hebilicious changed the title feat: add edge support to aws-lambda preset feat: add edge support to aws-lambda preset (WIP) Aug 14, 2023
@pi0 pi0 changed the title feat: add edge support to aws-lambda preset (WIP) feat: add edge support to aws-lambda presets Aug 20, 2023
import { Nitro } from "../types";
import { writeFile } from "../utils";

export async function generateCdkApp(nitro: Nitro) {
Copy link
Contributor Author

@Hebilicious Hebilicious Aug 27, 2023

Choose a reason for hiding this comment

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

This is more verbose and error prone than SST, harder to test and doesn't handle regular lambdas.

super.validateBuildOutput();
}

protected createFunctionForRegional(): CdkFunction {
Copy link

@jdevdevdev jdevdevdev Nov 1, 2023

Choose a reason for hiding this comment

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

This template may need to be updated for the changes that were made to SST .

It has been updated in my test repo.

A faster way to merging might be to split SST integration and the lambda edge preset code into different PRs. (Assuming adding a dependency is what is the reservation here.)

Let the user build the deployment and add deployment guides / documentation on nitro.unjs.io / nuxt.com.

@Hebilicious Hebilicious mentioned this pull request Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws lambda-edge
2 participants