feat: Changed Sourcemap default value to false #375
Conversation
github-actions
bot
added
pr/external
stage/needs-triage
lucashuy
added
area/workflow/node_npm
pr/internal
area/build
and removed
pr/external
stage/needs-triage
mildaniel
left a comment
mildaniel
left a comment
There was a problem hiding this comment.
LGTM! But we also have integration tests inside of SAM-CLI. Make sure you change those accordingly as well when making these types of changes.
| The Lambda builder invokes `esbuild` with sensible defaults that will work for the majority of cases. Importantly, the following parameters are set by default | ||
|
|
||
| * `--minify`, as it [produces a smaller runtime package](https://esbuild.github.io/api/#minify) | ||
| * `--sourcemap`, as it generates a [source map that allows for correct stack trace reporting](https://esbuild.github.io/api/#sourcemap) in case of errors (see the [Error reporting](#error-reporting) section above) |
There was a problem hiding this comment.
does removing a default setting cause any breaking changes? what happens to an app which did not bother about setting any defaults, do we see a difference in the artifacts being built?
aws/aws-sam-cli#4062 indicates that enabling source map becomes an either or instead of both.
There was a problem hiding this comment.
On it's own, setting the source map option (or previously with it being True by default) would only generate the source map file. To actually use it, customers would have to also provide NODE_OPTIONS: --enable-source-maps. With this change to default False, it would technically cause a breaking change without the PR in SAM CLI. Lambda builders would no longer include source map file by default.
You're right on with the PR in SAM CLI, that PR is the other side to this one where the customer only has to provide either or. Once both these changes are released and leave the develop branch, customers shouldn't see any changes to their existing setups since they would already have set Sourcemap: true, NODE_OPTIONS or both and the changes in SAM CLI fills in the gaps so they can use source maps.
There was a problem hiding this comment.
Ok thanks, so these are closely related and need to be released together.
There was a problem hiding this comment.
Since this could be a considering a breaking change with just aws lambda builders in mind (if someone were to use it directly as a binary instead of via aws-sam-cli), we should think of upping the protocol version? though the protocol version depends on the arguments passed in to the workflow. Do we know of any other tools that maybe using the aws-lambda-builders directly apart from sam cli?
sriram-mv
left a comment
sriram-mv
left a comment
There was a problem hiding this comment.
I'm approving with the caveat that this is experimental, we should have a mechanism in place such that we do not make such kind of breaking changes as there could be other consumers of aws lambda builders apart from aws sam cli.
3 participants
Related to AWS SAM CLI change: aws/aws-sam-cli#4062
Description of changes:
This change changes source map's default value from
truetofalse.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.