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(layers): add createRequire banner in esm #2232

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

dreamorosi
Copy link
Contributor

Description of your changes

This PR introduces a new step in the layer publisher workflow that should fix a bug affecting customers using the Powertools Lambda Layers with functions bundled using ESM format.

As described in the linked issue, when the above conditions are met, the function throws a runtime error with the stack trace pointing at the /opt/nodejs/node_modules, which is where the code for our utilities is stored when using Layers.

The fix mirrors the recommendation that we give to customers having to support the require keyword and adds a polyfill at the top of the file causing the issue.

The step adds this banner dynamically when the Lambda layers are built and it does so only on the ESM build and on one specific file, which is importing the AWS X-Ray SDK for Node.js. While this is not a great solution, it's the only one that I could come up with that will allow us to have the polyfill present only in this specific case.

Adding the polyfill to our source would cause it to appear also when bundling for CJS, which would lead to a different runtime error, this time due to the require keyword defined twice.

While this PR doesn't add any test, I have tested manually the resulting layer build with functions in CJS and ESM both with and without Lambda Layers and even when adding a banner to the function bundle, it appears to fix the issue.

By the end of this iteration, or the following at the latest, I'll work on adding ESM builds to our integration test matrix.

Related issues, RFCs

Issue number: #2231

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Mar 14, 2024
@dreamorosi dreamorosi requested review from a team as code owners March 14, 2024 22:09
@dreamorosi dreamorosi linked an issue Mar 14, 2024 that may be closed by this pull request
@boring-cyborg boring-cyborg bot added the layers Items related to the Lambda Layers pipeline label Mar 14, 2024
@pull-request-size pull-request-size bot added the size/S PR between 10-29 LOC label Mar 14, 2024
@github-actions github-actions bot added the bug Something isn't working label Mar 14, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@sthulb sthulb merged commit 730bcc9 into main Mar 15, 2024
11 checks passed
@sthulb sthulb deleted the 2231-bug-unable-to-use-lambda-layer-with-esm-bundles branch March 15, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working layers Items related to the Lambda Layers pipeline size/S PR between 10-29 LOC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: unable to use Lambda Layer with ESM bundles
2 participants