feat: Enable source maps if either Sourcemap: true or --enable-source-maps is provided #4062
Conversation
github-actions
bot
added
pr/external
stage/needs-triage
qingchm
removed
maintainer/need-followup
stage/needs-triage
| build_properties = metadata.get("BuildProperties", {}) | ||
| source_map = build_properties.get("Sourcemap", None) | ||
|
|
||
| if source_map and not node_option_set: |
There was a problem hiding this comment.
Based on the logic here it doesn't seem to have special handling to the scenario where node_option_set is true and the build_properties.get("Sourcemap") is an explicit value False, which I feel we might want to handle differently from it being None? Open to any reason for this self conflict input should not be a concern
There was a problem hiding this comment.
Yup, you're correct that it does not handle the case where NODE_OPTIONS is set and Sourcemap is false. In this case, no source map is generated, so the environment variable to enable source maps will not do anything even though it is set. The only potential concern the performance hit with source maps, I'm not quite sure if the performance is degraded with a source map file, or if its just solely by using the enable source map environment variable.
There was a problem hiding this comment.
Unsure of the performance concerns here, but to @qingchm 's question is there a change in behavior from before this PR and after? If there is a conflict, we can raise that as LOG warning that there is a conflict.
There was a problem hiding this comment.
(not @qingchm answering) Before this PR, setting Sourcemaps to false would effectively disable source map usage since no map file is generated, even if the env variable was provided, this was expected behaviour. The behaviour remains the same after the PR. The Sourcemap: false option is basically just a way to opt out of source maps if it was defined at the Global level.
|
Currently blocked awaiting further discussion/planning. |
| @staticmethod | ||
| def _warn_using_source_maps(): | ||
| click.secho( | ||
| "\nYou are using source maps, note that this comes with a performance hit!" |
There was a problem hiding this comment.
If they try to remove just the env var it will still be set next time if the source maps are still set in the metadata, right? Should we clarify that it needs to be removed from both spots?
There was a problem hiding this comment.
Nice catch, I'll update it to reflect this change
| source_map = build_properties.get("Sourcemap", None) | ||
|
|
||
| if source_map and not node_option_set: | ||
| LOG.debug( |
There was a problem hiding this comment.
Since we're mutating the customer template, I think it's more appropriate to make this an info level log.
…-maps is provided (aws#4062) * Added support to only specific Sourcemap or NODE_OPTIONS to enable source map usage * Added unit tests * Added warning message about improper NODE_OPTIONS usage * Fixed enabling source maps too late * Added missing space in string join * Improved clarity in enable source map function * Changed wording of variable from 'incorrect' to 'invalid' * Fixed comments and make pr * Adding missing newline * Changed warning message to make it clear they need to remove both options * Improved log message to include function name
5 participants
Related to lambda builders change: aws/aws-lambda-builders#375
Why is this change necessary?
Previously, both
Sourcemap: trueandNODE_OPTIONS: --enable-source-mapsneeded to be provided to enable source map usage. This is redundant since explicitly defining one would imply source map usage.How does it address the issue?
This change allows users to specify either
NODE_OPTIONS: --enable-source-mapsunder environment variables orSourcemap: trueunder esbuild build properties and have source maps enabled and added to their functions.NODE_OPTIONScan be defined at either the Globals level or the Function level.What side effects does this change have?
This change will modify the final template file in
sam buildby adding eitherSourcemaporNODE_OPTIONSat the function level.Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.