fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #13257

jogold · 2021-02-24T16:00:10Z

Refactor how esbuild is detected and run: check for a local installation
first and fallback to a global installation.

For local bundling, if a local version is detected, run esbuild using
the right package manager.

In Docker, always run the globally installed version.

Support for PNPM could now easily be added in another PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Refactor how `esbuild` is detected and run: check for a local version first and fallback to a global installation. For local bundling, if a local version is detected, run `esbuild` using the right package manager. In Docker, always run the globally installed version. Support for PNPM could now easily be added in another PR. Closes aws#13179

gitpod-io · 2021-02-24T16:00:16Z

andreialecu · 2021-02-25T09:21:09Z

@jogold I have a question about how this module should be used.

Say I have a lambdas folder underneath my main cdk project. Do I need to create a package.json and lockfile separately within it with dependencies?

That seems to be the case with the current implementation, at least with yarn v2.

It would make it easier if that was not required, but instead it could take dependencies from the main cdk project's package.json. But currently that does not work.

To clarify, the only setup I got to work currently was something like:

cdk/package.json <- lambdas should be able to use deps from here
cdk/yarn.lock
cdk/bin/...
cdk/stacks/... <- stacks here
cdk/stacks/lambdas/package.json
cdk/stacks/lambdas/various-lambdas.ts
cdk/stacks/lambdas/yarn.lock
cdk/stacks/lambdas/node_modules

This is a bit error prone because I need to remember to run yarn install within the lambdas folder when changing dependencies. It also requires a bit of extra consideration to the CI script.

jogold · 2021-02-25T09:28:26Z

@andreialecu

Do I need to create a package.json and lockfile separately within it with dependencies?

No, you can have infra and runtime code next to each other with a single package.json and lock file for your whole project. See examples in https://github.com/jogold/cloudstructs and https://github.com/jogold/cdk-s3-thumbnail. What doesn't work for you when you try this?

andreialecu · 2021-02-25T09:31:38Z

@jogold I just tried this again, and if I delete the package.json and related files from the lambdas directory I'm getting:

 > create-auth-challenge.ts: error: Could not resolve "crypto-secure-random-digit" (mark it as external to exclude it from the bundle)
    3 │ import { randomDigits } from "crypto-secure-random-digit";

The main package.json does include that dependency.

jogold · 2021-02-25T09:33:38Z

@jogold I just tried this again, and if I delete the package.json and related files from the lambdas directory I'm getting:
 > create-auth-challenge.ts: error: Could not resolve "crypto-secure-random-digit" (mark it as external to exclude it from the bundle)
    3 │ import { randomDigits } from "crypto-secure-random-digit";

Can you share a repo with this issue?

andreialecu · 2021-02-25T09:39:40Z

It seems that the problem is that this plugin would be needed:

https://github.com/yarnpkg/berry/tree/master/packages/esbuild-plugin-pnp

And unfortunately it only works with the build API, which cannot be used due to the aforementioned jsii issues.

I guess this will need to be mentioned as a caveat, that an intermediate package.json that uses plain node_modules needs to be used. Perhaps a CLI setting could be added to esbuild in order to specify plugins that way.

jogold · 2021-02-25T12:39:21Z

It seems that the problem is that this plugin would be needed:

OK, does this means that this PR is currently useless because even if we correctly detect the presence of esbuild, it won't be able to resolve the modules when bundling in a PnP setup?

andreialecu · 2021-02-25T13:12:54Z

@jogold not necessarily, no.

The setup I described above works. It just needs an additional package.json and node_modules within the lambdas folder.

Also I believe that evanw/esbuild#884 might be a small change and I think it has the potential to be implemented in esbuild.

Alternatively, https://yarnpkg.com/features/protocols/#patch can be used to patch esbuild and force it to load the pnp plugin. It would be a minimal patch.

So this PR would still be a prerequisite for getting either of the two options above to work.

andreialecu · 2021-02-27T17:18:17Z

@jogold would it be feasible to allow overriding the builder, optionally, via a callback?

Here's an example of what I mean:

	import {build} from "esbuild";

    const cognitoEventsLambda = new lambda.NodejsFunction(this, id, {
      entry: path.resolve(__dirname, "cognito-lambdas", filename),
      handler: "handler",
      role: lambdaRole,
      builder: async (config) => {
         return await build({
           ...config,
           plugins: [pnpPlugin()],
         });
      }
    });

builder can be optional, and if specified, it is used instead of spawning the CLI here: https://github.com/aws/aws-cdk/pull/13257/files#diff-cd86fbd4f2bbefcbcffc2143adccabafa1debe5981edbcdfcc766b5a705fe770R154-R171

It would pass the esbuild config as a parameter, which the client side function can just pass along to esbuild (or further modify).

This way it delegates to user land - and because esbuild is running there a peer dependency is no longer necessary, so everything would still work.

eladb · 2021-02-28T08:17:20Z

packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts

+    this.packageManager = PackageManager.fromLockFile(props.depsLockFilePath);
+
+    Bundling.esbuildInstallation = Bundling.esbuildInstallation ?? EsbuildInstallation.detect();
+    const runsLocally = !!Bundling.esbuildInstallation?.version.startsWith(ESBUILD_VERSION);


maybe esbuildInstallation?.isLocal

There are too many locals... this runsLocally here means local bundling (vs Docker bundling). The isLocal of the esbuildInstallation means is locally installed (vs globally installed).

eladb · 2021-02-28T08:21:22Z

packages/@aws-cdk/aws-lambda-nodejs/lib/esbuild-installation.ts

+    try {
+      try {
+        // Check local version first
+        const version = getModuleVersion('esbuild');


Why does this have to throw if the module doesn't exist? Can this be tryGetModuleVersion?

…cdk into lambda-nodejs-detect-esbuild

jogold · 2021-03-01T11:14:07Z

@jogold would it be feasible to allow overriding the builder, optionally, via a callback?

We could maybe let the user implement tryBundle() (or a esbuild bundling options enriched version of it) here

aws-cdk/packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts

Line 103 in ad01099

tryBundle(outputDir: string) {

(but it will have to be sync)

aws-cdk-automation · 2021-03-01T11:45:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 6b99302
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2021-04-13T08:47:06Z

@jogold sorry for the delay... Can you update this branch and let me know if it's ready for a review?

eladb · 2021-05-02T07:09:42Z

@jogold closing for now. reopen when ready to continue.

github-actions bot added the @aws-cdk/aws-lambda-nodejs label Feb 24, 2021

github-actions bot assigned eladb Feb 24, 2021

jogold mentioned this pull request Feb 24, 2021

(aws-lambda-nodejs): does not properly detect esbuild when used with Yarn 2+ in PnP mode #13179

Closed

jogold added 2 commits February 24, 2021 17:44

isLocal

af8e73c

isLocal in test

5f713ce

Merge branch 'master' into lambda-nodejs-detect-esbuild

4bc3964

andreialecu mentioned this pull request Feb 25, 2021

Specifying plugins via the CLI evanw/esbuild#884

Closed

eladb reviewed Feb 28, 2021

View reviewed changes

jogold added 3 commits March 1, 2021 12:05

Merge branch 'master' into lambda-nodejs-detect-esbuild

ce588e1

tryGetModuleVersion

1cd295a

Merge branch 'lambda-nodejs-detect-esbuild' of github.com:jogold/aws-…

6b99302

…cdk into lambda-nodejs-detect-esbuild

eladb closed this May 2, 2021

jogold mentioned this pull request May 18, 2021

fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #14739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #13257

fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #13257

jogold commented Feb 24, 2021 •

edited

Loading

gitpod-io bot commented Feb 24, 2021 •

edited

Loading

andreialecu commented Feb 25, 2021 •

edited

Loading

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021 •

edited

Loading

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021

andreialecu commented Feb 27, 2021 •

edited

Loading

eladb Feb 28, 2021

jogold Mar 1, 2021 •

edited

Loading

eladb Feb 28, 2021

jogold commented Mar 1, 2021

aws-cdk-automation commented Mar 1, 2021

eladb commented Apr 13, 2021

eladb commented May 2, 2021

fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #13257

fix(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #13257

Conversation

jogold commented Feb 24, 2021 • edited Loading

gitpod-io bot commented Feb 24, 2021 • edited Loading

andreialecu commented Feb 25, 2021 • edited Loading

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021 • edited Loading

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021

jogold commented Feb 25, 2021

andreialecu commented Feb 25, 2021

andreialecu commented Feb 27, 2021 • edited Loading

eladb Feb 28, 2021

Choose a reason for hiding this comment

jogold Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

eladb Feb 28, 2021

Choose a reason for hiding this comment

jogold commented Mar 1, 2021

aws-cdk-automation commented Mar 1, 2021

AWS CodeBuild CI Report

eladb commented Apr 13, 2021

eladb commented May 2, 2021

jogold commented Feb 24, 2021 •

edited

Loading

gitpod-io bot commented Feb 24, 2021 •

edited

Loading

andreialecu commented Feb 25, 2021 •

edited

Loading

andreialecu commented Feb 25, 2021 •

edited

Loading

andreialecu commented Feb 27, 2021 •

edited

Loading

jogold Mar 1, 2021 •

edited

Loading