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

aws lambda build issue with esbuild v0.22.0 #3819

Closed
volodyad opened this issue Jul 1, 2024 · 21 comments · Fixed by #3820
Closed

aws lambda build issue with esbuild v0.22.0 #3819

volodyad opened this issue Jul 1, 2024 · 21 comments · Fixed by #3820

Comments

@volodyad
Copy link

volodyad commented Jul 1, 2024

Aws cdk generates next command on lambda build via esbuild

npx esbuild --bundle path/index.ts --target=node20 --platform=node --outfile=test-index.js '--external:@aws-sdk/*'

with version 0.22.0 it makes build with unbundled dependencies, as aws expect to bundle by default - builds are failing

with version v0.21.0 all works as expected.
If you build lambda via default docker is uses next command "npm install -g esbuild@0" to install latest esbuild, so literally all lambda builds via aws docker are failing

@VanTanev
Copy link

VanTanev commented Jul 1, 2024

Hey @evanw,

Just wanted to share that this affects us as well, and it will be affecting any aws-cdk lambda user. Unfortunately, while this one is on aws-cdk for installing esbuild@0 here the fastest fix may be to revert the breaking change that makes --packages=external the default behavior of esbuild.

Of course, that decision is entirely up to you.

The related aws-cdk issue: https://github.com/aws/aws-cdk/issues/30717 (edited to a string link because people coming from the aws-cdk issue apparently think it's fine to shout here)

@martzcodes
Copy link

Why was an EXPERIMENTAL deliberate breaking change introduced as a minor version bump?!?

@VanTanev
Copy link

VanTanev commented Jul 1, 2024

Why was an EXPERIMENTAL deliberate breaking change introduced as a minor version bump?!?

This is the contract of a 0.x.y semver. The issue is with aws-cdk installing esbuild@0 and not pinning to a known compatible version.

@martzcodes
Copy link

martzcodes commented Jul 1, 2024

True, and I forgot about the 0.x quirks of npm, but for a project that has 32 million weekly installs a bit more care should be taken.
CDK doesn't install esbuild by default (if not installed it uses docker which uses it, but that's super slow because of docker so many avoid it)...
on init'ing a project many CDK users will then do npm install esbuild --save-dev as one of their first steps, which would automatically pull in this experimental breaking change

@khastation
Copy link

Can report the same, 0.22 kills bundling of aws lambda with cdk, leading to module not found errors. I've pinned esbuild to 0.21.5 to fix for now.

@NickIannelli
Copy link

The treatment of esbuild being on a 0.xx release and sitting behind that as an excuse to making breaking changes without any form of input from the community, while getting almost 26 million downloads a week is abysmal behaviour from the maintainer.

Whether you agree with your software being stable or beta doesn't really matter when it's deeply relied on by millions of users.

The decision to release a breaking change without even a second thought intentionally (as indicated by the release notes) for a reason essentially being "people are confusing the initial intent, so I'm breaking their work." is genuinely baffling.

"many people don't read the documentation and don't do this, and are then confused when it doesn't work"

What about the millions of people that this already was working for?

This is something that could've been done progressively with ease.

  1. Start warning when no --packages option is enabled that the default behaviour will be changing with the next release, and a planned date
  2. Release the breaking change after the warning has existed for at least a week, preferably a month. At least people will have been warned and had an opportunity to update their default behaviours

But no, you took the easy way out and sit behind the "it's beta software, we can break things" mask as an excuse for poor practices.

@kennu
Copy link

kennu commented Jul 2, 2024

Having the same ImportModuleError problem with CDK Lambda functions. Really bad, because there's no warning at build/deployment time. Lambda functions just stop working after redeploying with esbuild 0.22 and it's not so easy to track down the reason.

@evanw
Copy link
Owner

evanw commented Jul 2, 2024

It sounds like the primary problem here is caused by a very large company locking their widely-used product to essentially esbuild@latest, which has been very explicitly not the recommended way to use esbuild since the start of the esbuild project. This is the reason the very first paragraph in esbuild's getting started documentation tells you to install esbuild with the --save-exact flag:

First, download and install the esbuild command locally. A prebuilt native executable can be installed using npm (which is automatically installed when you install the node JavaScript runtime):

npm install --save-exact --save-dev esbuild

This is also the reason why every single breaking change release for the past four years (from 0.8.0 to 0.22.0) has had a notice like this as the very first line of the release notes:

This release deliberately contains backwards-incompatible changes. To avoid automatically picking up releases like this, you should either be pinning the exact version of esbuild in your package.json file (recommended) or be using a version range syntax that only accepts patch upgrades such as ^0.21.0 or ~0.21.0. See npm's documentation about semver for more information.

The way I've been getting feedback on things since the start of the project has been to publish a breaking change release to avoid disrupting people, and then get feedback on that new release line. This also isn't some random breaking change for fun. It's a change that I've been thinking about for a while, and it solves a long-running class of issues that people encounter when they use esbuild as you can see from the many issues that I keep getting about it. I linked to some of them in the recent release notes:

I was not previously aware that Amazon was using an unpinned version of esbuild for a widely-used product of theirs. For what it's worth, I don't believe Amazon has ever communicated with me about the use of esbuild in their product. I'm sorry about the disruption that this change has caused. I sincerely hope that Amazon learns to pin their dependencies in the future to avoid breaking their customers.

Perhaps the best path forward here could be for esbuild to use a beta channel model for further development, to make sure that npm install esbuild@latest is a stable esbuild release that has already been widely tested instead of being the latest version of esbuild like it is now. I'll have to learn more about how npm tags work first though as I haven't set up that kind of workflow before.

@HanabishiRecca
Copy link

I'm sorry about the disruption that this change has caused.

You shouldn't be sorry really. Amazon makes money on your project instead of you. If I were you, I would have said "f**k your AWS, if you want it your way, then pay me".

And don't listen to the whiners above. Who also probably earn money on free project and still dare to criticize.

@prisis
Copy link

prisis commented Jul 2, 2024

They did fix it in aws/aws-cdk@7f5ce4b.

This (esbuild) project follows semver, in the manifest it described how semver works, aws has a prob and a docker arg to change the version too, so there was no issue with the release of esbuild.

In the end someone would still complain if you would release a v2 (in this case v0.22.0) with a breaking change...

@kennu
Copy link

kennu commented Jul 2, 2024

In my view, the whole 0.x numbering scheme is not such a good idea in general. It would be better to release major versions on breaking changes. I don't really see the benefit in having 0.x versions, but I see the problems.

@lazarljubenovic
Copy link

In my view, the whole 0.x numbering scheme is not such a good idea in general. It would be better to release major versions on breaking changes. I don't really see the benefit in having 0.x versions, but I see the problems.

The QWERTY keyboard isn't a good idea in general either as it's designed to slow you down, and doesn't translate to the digital world at all, and yet here we are.

The 0.x.y exception to the break-at-major-only makes perfect sense though; otherwise every project would be at version 2103.0.0 by the time you hear of it. That's the benefit. The 0 at the start of the version is a signal enough that breaking changes are coming since it means, by definition, that the public API hasn't been finalized yet. esbuild is simply following a very well-known semver specification.

Don't install @0s, simple as that.

@HanabishiRecca
Copy link

Semver spec ignores leading zeroes and default npm save-prefix='^' would not allow minor bump in this case.
I.e. default ^0.21.x would never install 0.22 on its own. https://semver.otterlord.dev/?package=esbuild&range=^0.21.0

And If you use latest, *, 0 etc., as AWS did, the version number doesn't do anything anyway.

@kennu
Copy link

kennu commented Jul 2, 2024

The 0.x.y exception to the break-at-major-only makes perfect sense though; otherwise every project would be at version 2103.0.0 by the time you hear of it. That's the benefit.

For me, version 2103.0.0 is completely fine, if it would prevent problems like the present one. Versions exist to help people, not other way round.

@lazarljubenovic
Copy link

The 0.x.y exception to the break-at-major-only makes perfect sense though; otherwise every project would be at version 2103.0.0 by the time you hear of it. That's the benefit.

For me, version 2103.0.0 is completely fine, if it would prevent problems like the present one. Versions exist to help people, not other way round.

Another thing that prevents problems is following the spec that says that 0.22.0 is a breaking change from 0.21.0. People are pretending like it's rocket science. Heck, leap years are more complicated than this.

@kennu
Copy link

kennu commented Jul 2, 2024

Another thing that prevents problems is following the spec that says that 0.22.0 is a breaking change from 0.21.0. People are pretending like it's rocket science. Heck, leap years are more complicated than this.

I agree, both are good ways to prevent problems. I think 0.x versions tend to cause this kind of problems in many places, and you can try to educate people, and you can also try to avoid using 0.x versions unless really required.

alex9smith added a commit to govuk-one-login/di-account-management-backend that referenced this issue Jul 3, 2024
This effectively reverts #259 and #266 as the write-activity-log lambda
is failing all requests after them.

The update to ESbuild isn't actually a minor update - going to 0.22.0
breaks how packages are built. See their changelog
(https://github.com/evanw/esbuild/blob/main/CHANGELOG.md) and related
issues (evanw/esbuild#3819) for more details.

We did already update to ESBuild 0.23.0 which should have solved the
problem, but we're still seeing all invocations failing. Let's revert
back to a known good config while we understand what's happen, but we're
still seeing all invocations failing. Let's revert back to a known good
config while we understand what's happened.
alex9smith added a commit to govuk-one-login/di-account-management-backend that referenced this issue Jul 3, 2024
## Proposed changes

<!-- Provide a summary of your changes in the title above -->
<!-- Include the Jira ticket number in square brackets as prefix, eg
`[OLH-XXXX]: PR Title` -->

### What changed

Revert ESbuild and AWS SDK updates. This effectively reverts #259 and
#266 as the write-activity-log lambda is failing all requests after
them.


### Why did it change

The update to ESbuild isn't actually a minor update - going to 0.22.0
breaks how packages are built. See their changelog
(https://github.com/evanw/esbuild/blob/main/CHANGELOG.md) and related
issues (evanw/esbuild#3819) for more details.

We did already update to ESBuild 0.23.0 which should have solved the
problem, but we're still seeing all invocations failing. Let's revert
back to a known good config while we understand what's happened.

### Related links

<!-- List any related PRs -->
<!-- List any related ADRs or RFCs -->

## Checklists

<!-- Merging this PR deploys to production. Please answer accurately.
-->

### Environment variables or secrets

- [x] No environment variables or secrets were added or changed

### How to review

Please check the version numbers here against the ones in
#259
@cortexcompiler
Copy link

Why was an EXPERIMENTAL deliberate breaking change introduced as a minor version bump?!?

This is the contract of a 0.x.y semver. The issue is with aws-cdk installing esbuild@0 and not pinning to a known compatible version.

I just want to respond to this, as I think a breaking change should be a major release increment. I feel that the npm documentation agrees with this:
https://docs.npmjs.com/about-semantic-versioning#incrementing-semantic-versions-in-published-packages

Minor Release: "Backward compatible new features" (Increment the middle digit)
Major Release: "Changes that break backward compatibility" (Increment the first digit)

So introducing breaking changes in a minor release does break this paradigm and will hurt folks that pin the major release. Note that this would not have helped the CDK issue.

image

@HanabishiRecca
Copy link

So introducing breaking changes in a minor release does break this paradigm and will hurt folks that pin the major release.

Except such folks shouldn't exist. Because any sane version wildcard (^0.21.x, ~0.21.x, 0.21.*, 0.21...) pins the minor release in this case.

@VanTanev
Copy link

VanTanev commented Jul 9, 2024

Why was an EXPERIMENTAL deliberate breaking change introduced as a minor version bump?!?

This is the contract of a 0.x.y semver. The issue is with aws-cdk installing esbuild@0 and not pinning to a known compatible version.

I just want to respond to this, as I think a breaking change should be a major release increment. I feel that the npm documentation agrees with this: https://docs.npmjs.com/about-semantic-versioning#incrementing-semantic-versions-in-published-packages

You're wrong, read the whole spec: https://semver.org/#spec-item-4

Major version zero (0.y.z) is for initial development. 
Anything MAY change at any time. 
The public API SHOULD NOT be considered stable.

@RobinTail
Copy link

@evanw , Maybe it's time for esbuild to go with version 1 and do the next breaking changes in 2 and so on?

image

https://semver.org/#how-do-i-know-when-to-release-100

@RobinTail
Copy link

@evanw , what do you think on my suggestion?

#3819 (comment)

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 a pull request may close this issue.