feat(aws): Create unified lambda layer for ESM and CJS #17012
Conversation
| 'build/aws/dist-serverless/nodejs/node_modules/@sentry/aws-serverless/build/npm/cjs/index.js', | ||
| ); | ||
| console.log('Installing local @sentry/aws-serverless into build/aws/dist-serverless/nodejs.'); | ||
| run('npm install . --prefix ./build/aws/dist-serverless/nodejs --install-links --silent'); |
There was a problem hiding this comment.
Should we use yarn here, for consistency? 🤔 no strong feelings, just thinking about it...
There was a problem hiding this comment.
also, I think we can use --production here possibly too, to avoid even installing dev dependencies...?
There was a problem hiding this comment.
Should we use yarn here, for consistency? 🤔 no strong feelings, just thinking about it...
I couldn't find an equivalent yarn command. There is yarn file:/some/path but from my quick testing this installs the root sentry-javascript package instead of just the one package. (Also there is no --prefix equivalent in yarn v1)
also, I think we can use
--productionhere possibly too, to avoid even installing dev dependencies...?
I tried that but it didn't really work for some reason, node_modules was still >50MB.
|
|
||
| console.log('Cleaning up empty directories.'); | ||
|
|
||
| removeEmptyDirs('./build/aws/dist-serverless/nodejs/node_modules'); |
There was a problem hiding this comment.
is it important/necessary that we clean up empty dirs? Just wondering...
Alternate idea, not sure if this makes sense: instead of deleting the files that are not in nft, could we instead just copy the files that are traced by nft into a final place? 🤔
There was a problem hiding this comment.
is it important/necessary that we clean up empty dirs? Just wondering...
Not strictly necessary, just saves a few more bytes I guess...
Alternate idea, not sure if this makes sense: instead of deleting the files that are not in nft, could we instead just copy the files that are traced by nft into a final place? 🤔
This was the original idea behind #3110 but was later abandoned in #5146 since it was too unreliable (and way more complex).
|
|
||
| removeEmptyDirs('./build/aws/dist-serverless/nodejs/node_modules'); | ||
|
|
||
| await minifyJavaScriptFiles(fileList); |
There was a problem hiding this comment.
I'm not sure how good of an idea it is to do this manually here... 🤔 could we instead leverage a regular rollup build for this step, somehow?
There was a problem hiding this comment.
to be clear, I am not saying we def. need to do that, just that we should think about it :D what about other things our rollup build does, other than minifying, ...?
There was a problem hiding this comment.
I'm not sure how good of an idea it is to do this manually here...
yeah I agree, already encountered some issues with otel. I removed it for now, might tackle later in a follow-up.
msonnb
force-pushed
the
ms/esm-lambda-layer
branch
2 times, most recently
from
300b155 to
88d24b0
Compare
feat(aws): Create unified lambda layer for ESM and CJS
msonnb
force-pushed
the
ms/esm-lambda-layer
branch
from
88d24b0 to
5a9af39
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Build Script Crashes on Missing Files
The fs.unlinkSync() calls for package.json and package-lock.json lack error handling. If these files are missing, the build script will crash. These operations should either be wrapped in try-catch blocks or use fs.rmSync() with force: true.
packages/aws-serverless/scripts/buildLambdaLayer.ts#L26-L28
sentry-javascript/packages/aws-serverless/scripts/buildLambdaLayer.ts
Lines 26 to 28 in 5a9af39
Was this report helpful? Give feedback by reacting with 👍 or 👎
This introduces a new AWS lambda layer that supports both ESM and CJS. Instead of bundling the whole SDK, we install the local NPM package and then prune all not strictly necessary files from
node_modulesby using@vercel/nftto keep the layer size as small as possible.closes #16876
closes #16883
closes #16886
closes #16879