-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 #13257
Conversation
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
@jogold I have a question about how this module should be used. Say I have a 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 To clarify, the only setup I got to work currently was something like:
This is a bit error prone because I need to remember to run |
No, you can have infra and runtime code next to each other with a single |
@jogold I just tried this again, and if I delete the
The main |
Can you share a repo with this issue? |
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 I guess this will need to be mentioned as a caveat, that an intermediate |
OK, does this means that this PR is currently useless because even if we correctly detect the presence of |
@jogold not necessarily, no. The setup I described above works. It just needs an additional Also I believe that evanw/esbuild#884 might be a small change and I think it has the potential to be implemented in Alternatively, https://yarnpkg.com/features/protocols/#patch can be used to patch So this PR would still be a prerequisite for getting either of the two options above to work. |
@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()],
});
}
});
It would pass the This way it delegates to user land - and because |
this.packageManager = PackageManager.fromLockFile(props.depsLockFilePath); | ||
|
||
Bundling.esbuildInstallation = Bundling.esbuildInstallation ?? EsbuildInstallation.detect(); | ||
const runsLocally = !!Bundling.esbuildInstallation?.version.startsWith(ESBUILD_VERSION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe esbuildInstallation?.isLocal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
try { | ||
try { | ||
// Check local version first | ||
const version = getModuleVersion('esbuild'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to throw if the module doesn't exist? Can this be tryGetModuleVersion
?
…cdk into lambda-nodejs-detect-esbuild
We could maybe let the user implement
(but it will have to be |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@jogold sorry for the delay... Can you update this branch and let me know if it's ready for a review? |
@jogold closing for now. reopen when ready to continue. |
Refactor how
esbuild
is detected and run: check for a local installationfirst and fallback to a global installation.
For local bundling, if a local version is detected, run
esbuild
usingthe 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