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(lambda-nodejs): esbuild detection with Yarn 2 in PnP mode #14739

Merged
merged 15 commits into from
May 19, 2021

Conversation

jogold
Copy link
Contributor

@jogold jogold commented May 18, 2021

Original PR #13257.

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 #13179


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

jogold and others added 9 commits February 24, 2021 16:53
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
Copy link

gitpod-io bot commented May 18, 2021

this.packageManager = PackageManager.fromLockFile(props.depsLockFilePath);

Bundling.esbuildInstallation = Bundling.esbuildInstallation ?? EsbuildInstallation.detect();
const mustRunInDocker = !Bundling.esbuildInstallation?.version.startsWith(ESBUILD_VERSION);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const mustRunInDocker = !Bundling.esbuildInstallation?.version.startsWith(ESBUILD_VERSION);
const mustRunInDocker = !Bundling.esbuildInstallation?.version.startsWith(`${ESBUILD_MAJOR_VERSION}.`);

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if perhaps we should just emit an error if the major version doesn't match, otherwise users will not understand why we fallback to docker.

import { BundlingOptions } from './types';
import { exec, extractDependencies, findUp, getEsBuildVersion, LockFile } from './util';
import { exec, extractDependencies, findUp } from './util';

const ESBUILD_VERSION = '0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ESBUILD_VERSION = '0';
const ESBUILD_MAJOR_VERSION = '0';

@jogold jogold mentioned this pull request May 19, 2021
2 tasks
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 221611d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb eladb enabled auto-merge (squash) May 19, 2021 10:38
@eladb eladb merged commit 5c84696 into aws:master May 19, 2021
jogold added a commit to jogold/aws-cdk that referenced this pull request May 28, 2021
…e with a NodejsFunction

In aws#14739 changed how `esbuild` is run. It now uses the package manager.

When consuming a node module with a `NodejsFunction` we need to run
`esbuild` from the directory containing the lock file. This is where
we have `node_modules\.bin`. If we run it from the directory containing
the entry file the package manager doesn't "see" `node_modules\.bin`.
mergify bot pushed a commit that referenced this pull request May 30, 2021
…e with a NodejsFunction (#14914)

#14739 changed how `esbuild` is run. It now uses the package manager.

When consuming a node module with a `NodejsFunction` we need to run
`esbuild` from the directory containing the lock file. This is where
we have `node_modules\.bin`. If we run it from the directory containing
the entry file the package manager doesn't "see" `node_modules\.bin`.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…e with a NodejsFunction (aws#14914)

aws#14739 changed how `esbuild` is run. It now uses the package manager.

When consuming a node module with a `NodejsFunction` we need to run
`esbuild` from the directory containing the lock file. This is where
we have `node_modules\.bin`. If we run it from the directory containing
the entry file the package manager doesn't "see" `node_modules\.bin`.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@jogold jogold deleted the lambda-nodejs-detect-esbuild branch September 29, 2021 12:30
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-nodejs): does not properly detect esbuild when used with Yarn 2+ in PnP mode
3 participants