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

refactor(deploy): aws-lambda serve #882

Merged
merged 12 commits into from
Sep 27, 2024
Merged

refactor(deploy): aws-lambda serve #882

merged 12 commits into from
Sep 27, 2024

Conversation

dai-shi
Copy link
Owner

@dai-shi dai-shi commented Sep 15, 2024

Copy link

vercel bot commented Sep 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Sep 27, 2024 3:15am

Copy link

codesandbox-ci bot commented Sep 15, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@dai-shi dai-shi changed the title refactor(build): aws-lambda serve refactor(deploy): aws-lambda serve Sep 15, 2024
@dai-shi
Copy link
Owner Author

dai-shi commented Sep 15, 2024

@aheissenberger Can you test this and see if it works?

@dai-shi dai-shi mentioned this pull request Sep 16, 2024
88 tasks
@rmarscher
Copy link
Contributor

I have been having trouble with the hono import in the built serve-aws-lambda.js file. It looks like it is not building hono/context into the bundle? Here is a section of the built serve-aws-lambda.js file:

const importHono = () => import("./assets/index-BxM3V2xC.js");
const importHonoContextStorage = () => import("hono/context-storage");
const importHonoNodeServerServeStatic = () => import("./assets/serve-static-CLws5J7J.js");
const importHonoAwsLambda = () => import("./assets/index-DM8TMISb.js");
const { Hono } = await importHono();
const { contextStorage } = await importHonoContextStorage();
const { serveStatic } = await importHonoNodeServerServeStatic();
const { handle } = await importHonoAwsLambda();

And then I get a runtime error on AWS when it tries to serve a page:
Cannot find package 'hono' imported from /var/task/serve-aws-lambda.js

I'm not sure if this is caused by the way I am trying to build this branch and copy to my AWS project dependencies. I have installed hono 4.6.1 in my AWS project.

@rmarscher
Copy link
Contributor

I was able to get it working by commenting out the hono context storage from the serve js content. So that seems to be the only issue. I don't understand how hono/aws-lambda is built correctly but hono/context-storage is not.

@rmarscher
Copy link
Contributor

@rmarscher
Copy link
Contributor

It works for me if I remove external: ['hono/context-storage'] from the vite config for buildServerBundle and use import { importHonoContextStorage } from "waku/unstable_hono"; in a server component.

import { importHonoContextStorage } from "waku/unstable_hono";

const getData = async () => {
  const { getContext } = await importHonoContextStorage();
  const c = getContext();
  const acceptHeader = c.req.header("Accept");
  return { acceptHeader };
}

I had to leave hono/context-storage as external in the dev-server-impl.ts though. I tried updating the runner in cli.ts to import from unstable_hono:

  const contextStorage = (await importHonoContextStorage()).contextStorage;
  app.use(contextStorage());

But that results in Context is not available error. Leaving cli.ts as is works in dev mode while still using importHonoContextStorage in the server component.

I don't personally have a use case for waku start but I have a feeling that might have issues because the hono context in the runner will be different from the one in the build. Maybe the cli could be changed to invoke runners that are part of the server build instead of having a separate runner that imports entries directly.

@rmarscher
Copy link
Contributor

I made a related documentation change in #897

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 24, 2024

@rmarscher Thanks for investigating this.

Oh right... probably this... 58f7ded#diff-275ea9f25448c4618b0f5d5e886b0f4072dad0167fc254e3804e46be65ec2096R249

If that's the problem, why does cloudflare version work?
Can you confirm if the generated file has const importHonoContextStorage = () => import("hono/context-storage"); as well?

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 24, 2024

And then I get a runtime error on AWS when it tries to serve a page:
Cannot find package 'hono' imported from /var/task/serve-aws-lambda.js

Does it work if you install hono manually for your app?

@aheissenberger
Copy link
Contributor

aheissenberger commented Sep 24, 2024

no - even after installing hono as a dependency the import of the hono/context-storage is not bundled by vite which might be a bug in rollup handling package / exports definitions - all others are included in the ./assets/directory:

const importHono = () => import("./assets/index-BxM3V2xC.js");
/*                                            vv no ./assets/..   */
const importHonoContextStorage = () => import("hono/context-storage");

const importHonoNodeServerServeStatic = () => import("./assets/serve-static-CLws5J7J.js");
const importHonoAwsLambda = () => import("./assets/index-DM8TMISb.js");
const { Hono } = await importHono();
const { contextStorage } = await importHonoContextStorage();
const { serveStatic } = await importHonoNodeServerServeStatic();
const { handle } = await importHonoAwsLambda();

here is a simple code to debug the problem without deployment to aws:

const requestHandlerPath = "./serve-aws-lambda.js";
const { handler } = await import(requestHandlerPath);
const eventData = {
  version: "2.0",
  routeKey: "$default",
  rawPath: "/",
  rawQueryString: "",
  headers: {
    accept: "*/*",
    "content-length": "0",
    host: "example.com",
    "user-agent": "PostmanRuntime/7.26.8",
    "x-amzn-trace-id": "Root=1-5f84c7a9-0e5b1e1e1e1e1e1e1e1e1e1e",
    "x-forwarded-for": "127.0.0.1",
    "x-forwarded-port": "443",
    "x-forwarded-proto": "https",
  },
  requestContext: {
    accountId: "123456789012",
    apiId: "api-id",
    domainName: "example.com",
    domainPrefix: "example",
    http: {
      method: "GET",
      path: "/",
      protocol: "HTTP/1.1",
      sourceIp: "127.0.0.1",
      userAgent: "PostmanRuntime/7.26.8",
    },
    requestId: "id",
    routeKey: "$default",
    stage: "$default",
    time: "12/Mar/2021:19:03:58 +0000",
    timeEpoch: 1615578238000,
  },
  isBase64Encoded: false,
};
const response = await handler(eventData, {});
console.log(response);

Solution

I was able to fix the broken vite resolution problem with this vite.config.ts:

import { defineConfig } from 'vite';
import path from "node:path";
export default defineConfig({
    resolve: {
        alias: {
          "hono/context-storage": path.resolve(__dirname, "node_modules/hono/dist/middleware/context-storage/index.js"),
        },
      },
});

But having the hono package installed as a dependency in the project is a must requirement!

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 24, 2024

It should be fixed with #898. Let me merge it.

@aheissenberger
Copy link
Contributor

It should be fixed with #898. Let me merge it.

do I understand correctly that this fix only ignores the rollup path resolution problem by catching and ignoring the error?

@aheissenberger
Copy link
Contributor

I suggest to rollback your fix and add this to the plugins e.g. vite-plugin-deploy-aws-lambda.ts (last 7 lines):

export function deployAwsLambdaPlugin(opts: {
  srcDir: string;
  distDir: string;
}): Plugin {
  const platformObject = unstable_getPlatformObject();
  let entriesFile: string;
  return {
    name: 'deploy-aws-lambda-plugin',
    config(viteConfig) {
      const { deploy, unstable_phase } = platformObject.buildOptions || {};
      if (unstable_phase !== 'buildServerBundle' || deploy !== 'aws-lambda') {
        return;
      }
      const { input } = viteConfig.build?.rollupOptions ?? {};
      if (input && !(typeof input === 'string') && !(input instanceof Array)) {
        input[SERVE_JS.replace(/\.js$/, '')] = `${opts.srcDir}/${SERVE_JS}`;
      }

     // fix for rollup resolution problem
      if (viteConfig.resolve===undefined) {
        viteConfig.resolve = {};
      }
      if (viteConfig.resolve.alias===undefined) {
        viteConfig.resolve.alias = {};
      }
      (viteConfig.resolve.alias as Record<string, string>)['hono/context-storage'] = path.resolve('node_modules/hono/dist/middleware/context-storage/index.js');

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 25, 2024

It should be fixed with #898. Let me merge it.

do I understand correctly that this fix only ignores the rollup path resolution problem by catching and ignoring the error?

No, my expectation is it should work flawlessly if users install hono in the app. It ignores if it's not installed.

@aheissenberger
Copy link
Contributor

What is the reason to set the ssr.external: ['hono/context-storage'], in the first place in build?

I think another fix is to remove external: ['hono/context-storage'], from the build config. It works for me if I comment out https://github.com/dai-shi/waku/blob/refactor/deploy/aws-lambda/packages/waku/src/lib/builder/build.ts#L249

That gets the output serve-aws-lambda.js to have

let contextStorage;
try {
  ({ contextStorage } = await import("./assets/index-ED7G-tsu.js"));
} catch {
}

you are quicker - I was just asking why this exists in the first place. I only tried to fix aws, as I have no knowledge about the other hosting platforms.

@rmarscher
Copy link
Contributor

What is the reason to set the ssr.external: ['hono/context-storage'], in the first place in build?

It was from a previous attempt to get the same module to be imported by the hono runtime and from server components. I'm pretty sure it can be safely removed. I'll give a try with Cloudflare. I have a few Waku apps. One on AWS Lambda and a few deployments on Cloudflare.

I use a fork of this adapter for AWS so I can use

const { streamHandle } = await importHonoAwsLambda();

and enable lambda streaming.

@rmarscher
Copy link
Contributor

I confirmed it also works great with cloudflare with external: ['hono/context-storage'] removed from build.ts.

@rmarscher
Copy link
Contributor

Here's my fork. It uses streaming and it separates the public assets from the lambda function. That way I don't include any non-needed static assets in the function bundle. I upload the public assets to an S3 bucket and use a fallback origin with cloudfront so to have it look there first before routing to the lambda function. That works very well. refactor/deploy/aws-lambda...rmarscher:waku:slimmer-aws-lambda-sst-patch

@aheissenberger
Copy link
Contributor

I use a fork of this adapter for AWS so I can use

const { streamHandle } = await importHonoAwsLambda();

and enable lambda streaming.

How about adding AWS Lambda Streaming as an option in the config file? Both variants are useful as AWS Streaming is not supported by the API Gateway.

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

What is the reason to set the ssr.external: ['hono/context-storage'], in the first place in build?

It's #894. Without it, it doesn't work with waku start (node server).
So, I think it's generally required.

But, I'm fine to remove it for aws-lambda deploy (and cloudflare too?)

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

@rmarscher Does d15b11b 👉 32e90a1 work?

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

I have no problems to run waku start with ssr.external: ['hono/context-storage'] removed in packages/waku/dist/lib/builder/build.js:200. I tried 03_demo and 08_cookies with the node server.

Hmmm? I just tried npm run examples:prd:08_cookies with removing the line, and it shows this error:

> waku start

ready: Listening on http://localhost:8080/
waku context [ 'count', '__waku_requestHeaders' ]
Error: Context is not available

@aheissenberger
Copy link
Contributor

I have no problems to run waku start with ssr.external: ['hono/context-storage'] removed in packages/waku/dist/lib/builder/build.js:200. I tried 03_demo and 08_cookies with the node server.

Hmmm? I just tried npm run examples:prd:08_cookies with removing the line, and it shows this error:

> waku start

ready: Listening on http://localhost:8080/
waku context [ 'count', '__waku_requestHeaders' ]
Error: Context is not available

yes - I get the same error - is it possible to limit ssr.external: ['hono/context-storage'] to the node server or do we know the need for ssr.external: ['hono/context-storage'] is also given for all other deployment adapters?

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

I'd go the other way around. The node server is the default and deploy adapters are options.

@aheissenberger
Copy link
Contributor

@dai-shi What is your suggestion to add an option for a deployment adapter in waku.config.ts to allow to activate AWS Lambda streaming?

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

@dai-shi What is your suggestion to add an option for a deployment adapter in waku.config.ts to allow to activate AWS Lambda streaming?

Good question. The config stuff is still something I'll be considering. So, I would like to delay the decision. Meanwhile, if you need some configurations in deployment adapters, the easiest way would be process.env... for now.

@aheissenberger
Copy link
Contributor

With latest commit both situations, with and without package hono installed work for example/03_demo and without any changes with 08_cookies deployed to aws.

@aheissenberger
Copy link
Contributor

Here's my fork. It uses streaming and it separates the public assets from the lambda function. That way I don't include any non-needed static assets in the function bundle. I upload the public assets to an S3 bucket and use a fallback origin with cloudfront so to have it look there first before routing to the lambda function. That works very well. refactor/deploy/aws-lambda...rmarscher:waku:slimmer-aws-lambda-sst-patch

@rmarscher when this refactoring is merged we can discuss to add streaming to this adapter. I also deploy the static assets to s3 and use cloudfront to cache and split the requests between waku and the static assent but do not need the output folder to be rearranged.

@rmarscher
Copy link
Contributor

I can confirm having this in the configResolved(config) section of the deploy plugin works.

      if (deploy === '[build output name]' && Array.isArray(config.ssr.external)) {
        config.ssr.external = config.ssr.external.filter(
          (item) => item !== 'hono/context-storage',
        );
      }

And resolveId(source) does not need

      if (source === 'hono/context-storage') {
        return { id: source, external: true };
      }

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 27, 2024

With latest commit both situations, with and without package hono installed work for example/03_demo and without any changes with 08_cookies deployed to aws.

Nice. Thanks for comfirning.

And resolveId(source) does not need

      if (source === 'hono/context-storage') {
        return { id: source, external: true };
      }

Ah, thanks.

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 27, 2024

Merging this.

@rmarscher Can you check the cloudflare one too and send a PR?

@rmarscher when this refactoring is merged we can discuss to add streaming to this adapter.

Nice. Looking forward to a PR based on you guys' discussions.

@dai-shi dai-shi merged commit abc5d17 into main Sep 27, 2024
28 checks passed
@dai-shi dai-shi deleted the refactor/deploy/aws-lambda branch September 27, 2024 04:12
@dai-shi dai-shi mentioned this pull request Sep 27, 2024
dai-shi added a commit that referenced this pull request Sep 27, 2024
Based on #882, this fixes some plugins
- Vercel
- Netlify (Found an issue: #909)
- Deno (Found an issue: #908)
dai-shi added a commit that referenced this pull request Sep 28, 2024
re: #882 (comment)
Applies the same fix used for handling hono/context-storage with aws
builds.

---------

Co-authored-by: daishi <daishi@axlight.com>
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