Skip to content

feat: Enable source maps if either Sourcemap: true or --enable-source-maps is provided #4062

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.

Sign up for GitHub

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

Merged
lucashuy merged 13 commits into from
Aug 3, 2022
Merged

feat: Enable source maps if either Sourcemap: true or --enable-source-maps is provided #4062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1dc156a
Added support to only specific Sourcemap or NODE_OPTIONS to enable so…
lucashuy Jul 14, 2022
24112f7
Added unit tests
lucashuy Jul 15, 2022
6528ad0
Added warning message about improper NODE_OPTIONS usage
lucashuy Jul 19, 2022
b16e6cc
Fixed enabling source maps too late
lucashuy Jul 21, 2022
25095ef
Added missing space in string join
lucashuy Jul 22, 2022
e5a017d
Merge branch 'develop' of https://github.com/aws/aws-sam-cli into ena…
lucashuy Jul 26, 2022
6d83423
Improved clarity in enable source map function
lucashuy Jul 26, 2022
ecdaedb
Changed wording of variable from 'incorrect' to 'invalid'
lucashuy Jul 26, 2022
12540c3
Fixed comments and make pr
lucashuy Jul 26, 2022
3798147
Adding missing newline
lucashuy Jul 26, 2022
6267ffa
Merge branch 'develop' into enable_source_maps
lucashuy Aug 3, 2022
6f4c2e9
Changed warning message to make it clear they need to remove both opt…
lucashuy Aug 3, 2022
7f4022b
Improved log message to include function name
lucashuy Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions samcli/commands/build/build_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ def run(self):
if is_sam_template:
SamApiProvider.check_implicit_api_resource_ids(self.stacks)

# modify the stack resources to support source maps
self._enable_source_maps()

try:
builder = ApplicationBuilder(
self.get_resources_to_build(),
Expand Down Expand Up @@ -265,6 +268,7 @@ def run(self):
stack, self._stack_name, self.build_dir, modified_template, build_result
)
modified_template = nested_stack_manager.generate_auto_dependency_layer_stack()

move_template(stack.location, output_template_path, modified_template)

click.secho("\nBuild Succeeded", fg="green")
Expand Down Expand Up @@ -307,6 +311,108 @@ def run(self):
wrapped_from = deep_wrap if deep_wrap else ex.__class__.__name__
raise UserException(str(ex), wrapped_from=wrapped_from) from ex

def _enable_source_maps(self):
"""
Appends ``NODE_OPTIONS: --enable-source-maps``, if Sourcemap is set to true
and sets Sourcemap to true if ``NODE_OPTIONS: --enable-source-maps`` is provided.
"""
using_source_maps = False
invalid_node_option = False

for stack in self.stacks:
for name, resource in stack.resources.items():
metadata = resource.get("Metadata", {})
if metadata.get("BuildMethod", "") != "esbuild":
continue

node_option_set = self._is_node_option_set(resource)

# check if Sourcemap is provided and append --enable-source-map if not set
build_properties = metadata.get("BuildProperties", {})
source_map = build_properties.get("Sourcemap", None)

if source_map and not node_option_set:
Copy link
Contributor

@qingchm qingchm Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lucashuy lucashuy Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sriram-mv sriram-mv Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lucashuy lucashuy Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

LOG.info(
"\nSourcemap set without --enable-source-maps, adding"
" --enable-source-maps to function %s NODE_OPTIONS",
name,
)

resource.setdefault("Properties", {})
resource["Properties"].setdefault("Environment", {})
resource["Properties"]["Environment"].setdefault("Variables", {})
existing_options = resource["Properties"]["Environment"]["Variables"].setdefault("NODE_OPTIONS", "")

# make sure the NODE_OPTIONS is a string
if not isinstance(existing_options, str):
invalid_node_option = True
else:
resource["Properties"]["Environment"]["Variables"]["NODE_OPTIONS"] = " ".join(
[existing_options, "--enable-source-maps"]
)

using_source_maps = True

# check if --enable-source-map is provided and append Sourcemap: true if it is not set
if source_map is None and node_option_set:
LOG.info(
"\n--enable-source-maps set without Sourcemap, adding Sourcemap to"
" Metadata BuildProperties for %s",
name,
)

resource.setdefault("Metadata", {})
resource["Metadata"].setdefault("BuildProperties", {})
resource["Metadata"]["BuildProperties"]["Sourcemap"] = True

using_source_maps = True

if using_source_maps:
self._warn_using_source_maps()

if invalid_node_option:
self._warn_invalid_node_options()

@staticmethod
def _is_node_option_set(resource: Dict) -> bool:
"""
Checks if the template has NODE_OPTIONS --enable-source-maps set

Parameters
----------
resource : Dict
The resource dictionary to lookup if --enable-source-maps is set

Returns
-------
bool
True if --enable-source-maps is set, otherwise false
"""
try:
node_options = resource["Properties"]["Environment"]["Variables"]["NODE_OPTIONS"]

return "--enable-source-maps" in node_options.split()
except (KeyError, AttributeError):
return False

@staticmethod
def _warn_invalid_node_options():
click.secho(
"\nNODE_OPTIONS is not a string! As a result, the NODE_OPTIONS environment variable will "
"not be set correctly, please make sure it is a string. "
"Visit https://nodejs.org/api/cli.html#node_optionsoptions for more details.\n",
fg="yellow",
)

@staticmethod
def _warn_using_source_maps():
click.secho(
"\nYou are using source maps, note that this comes with a performance hit!"
Copy link
Contributor

@mildaniel mildaniel Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@lucashuy lucashuy Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, I'll update it to reflect this change

" Set Sourcemap to false and remove"
" NODE_OPTIONS: --enable-source-maps to disable source maps.\n",
fg="yellow",
)

@staticmethod
def gen_success_msg(artifacts_dir: str, output_template_path: str, is_default_build_dir: bool) -> str:

Expand Down
178 changes: 178 additions & 0 deletions tests/unit/commands/buildcmd/test_build_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ def __init__(
self.runtime = runtime


class DummyStack:
def __init__(self, resource):
self.resources = resource


class TestBuildContext__enter__(TestCase):
@patch("samcli.commands.build.build_context.get_template_data")
@patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
Expand Down Expand Up @@ -636,8 +641,10 @@ def test_must_print_remote_url_warning(
@patch("samcli.commands.build.build_context.move_template")
@patch("samcli.commands.build.build_context.get_template_data")
@patch("samcli.commands.build.build_context.os")
@patch("samcli.commands.build.build_context.BuildContext._enable_source_maps")
def test_run_sync_build_context(
self,
source_maps_mock,
os_mock,
get_template_data_mock,
move_template_mock,
Expand Down Expand Up @@ -892,9 +899,11 @@ class TestBuildContext_run(TestCase):
@patch("samcli.commands.build.build_context.move_template")
@patch("samcli.commands.build.build_context.get_template_data")
@patch("samcli.commands.build.build_context.os")
@patch("samcli.commands.build.build_context.BuildContext._enable_source_maps")
def test_run_build_context(
self,
auto_dependency_layer,
source_map_mock,
os_mock,
get_template_data_mock,
move_template_mock,
Expand Down Expand Up @@ -947,6 +956,8 @@ def test_run_build_context(
]
nested_stack_manager_mock.return_value = given_nested_stack_manager

source_map_mock.side_effect = [modified_template_root, modified_template_child]

with BuildContext(
resource_identifier="function_identifier",
template_file="template_file",
Expand Down Expand Up @@ -1060,10 +1071,12 @@ def test_run_build_context(
@patch("samcli.commands.build.build_context.move_template")
@patch("samcli.commands.build.build_context.get_template_data")
@patch("samcli.commands.build.build_context.os")
@patch("samcli.commands.build.build_context.BuildContext._enable_source_maps")
def test_must_catch_known_exceptions(
self,
exception,
wrapped_exception,
source_map_mock,
os_mock,
get_template_data_mock,
move_template_mock,
Expand Down Expand Up @@ -1139,8 +1152,10 @@ def test_must_catch_known_exceptions(
@patch("samcli.commands.build.build_context.move_template")
@patch("samcli.commands.build.build_context.get_template_data")
@patch("samcli.commands.build.build_context.os")
@patch("samcli.commands.build.build_context.BuildContext._enable_source_maps")
def test_must_catch_function_not_found_exception(
self,
source_map_mock,
os_mock,
get_template_data_mock,
move_template_mock,
Expand Down Expand Up @@ -1262,3 +1277,166 @@ def test_check_exclude_warning(self, resource_id, exclude, should_print, log_moc
log_mock.warning.assert_called_once_with(BuildContext._EXCLUDE_WARNING_MESSAGE)
else:
log_mock.warning.assert_not_called()


class TestBuildContext_is_node_option_set(TestCase):
@parameterized.expand(
[
(
{"Properties": {"Environment": {"Variables": {"NODE_OPTIONS": "--enable-source-maps"}}}},
True,
),
(
{"Properties": {"Environment": {"Variables": {"NODE_OPTIONS": "nothing"}}}},
False,
),
]
)
def test_is_node_option_set(self, resource, expected_result):
build_context = BuildContext(
resource_identifier="resource_id",
template_file="template_file",
base_dir="base_dir",
build_dir="build_dir",
cache_dir="cache_dir",
cached=False,
parallel=False,
mode="mode",
)

self.assertEqual(build_context._is_node_option_set(resource), expected_result)

def test_enable_source_map_missing(self):
build_context = BuildContext(
resource_identifier="resource_id",
template_file="template_file",
base_dir="base_dir",
build_dir="build_dir",
cache_dir="cache_dir",
cached=False,
parallel=False,
mode="mode",
)

self.assertFalse(build_context._is_node_option_set({"Properties": {}}))


class TestBuildContext_enable_source_maps(TestCase):
@parameterized.expand(
[
({"test": {"Metadata": {"BuildMethod": "esbuild", "BuildProperties": {"Sourcemap": True}}}},),
(
{
"test": {
"Properties": {"Environment": {"Variables": {"NODE_OPTIONS": "--something"}}},
"Metadata": {"BuildMethod": "esbuild", "BuildProperties": {"Sourcemap": True}},
}
},
),
]
)
@patch("samcli.commands.build.build_context.BuildContext._is_node_option_set")
def test_enable_source_maps_only_source_map(self, resource, is_enable_map_mock):
build_context = BuildContext(
resource_identifier="resource_id",
template_file="template_file",
base_dir="base_dir",
build_dir="build_dir",
cache_dir="cache_dir",
cached=False,
parallel=False,
mode="mode",
)

stack = DummyStack(resource)
build_context._stacks = [stack]

is_enable_map_mock.return_value = False

build_context._enable_source_maps()

for _, resource in stack.resources.items():
self.assertIn("--enable-source-maps", resource["Properties"]["Environment"]["Variables"]["NODE_OPTIONS"])

@parameterized.expand(
[
({"test": {"Metadata": {"BuildMethod": "esbuild"}}}, True, True),
(
{
"test": {
"Properties": {"Environment": {"Variables": {"NODE_OPTIONS": "--enable-source-maps"}}},
"Metadata": {"BuildMethod": "esbuild"},
}
},
True,
True,
),
(
{
"test": {
"Metadata": {"BuildMethod": "esbuild", "BuildProperties": {"Sourcemap": False}},
}
},
True,
False,
),
]
)
@patch("samcli.commands.build.build_context.BuildContext._is_node_option_set")
@patch("samcli.commands.build.build_context.get_template_data")
def test_enable_source_maps_only_node_options(
self, resource, node_option_set, expected_value, get_template_mock, is_enable_map_mock
):
build_context = BuildContext(
resource_identifier="resource_id",
template_file="template_file",
base_dir="base_dir",
build_dir="build_dir",
cache_dir="cache_dir",
cached=False,
parallel=False,
mode="mode",
)

stack = DummyStack(resource)
build_context._stacks = [stack]

is_enable_map_mock.return_value = node_option_set

build_context._enable_source_maps()

for _, resource in stack.resources.items():
self.assertEqual(resource["Metadata"]["BuildProperties"]["Sourcemap"], expected_value)

@patch("samcli.commands.build.build_context.BuildContext._is_node_option_set")
@patch("samcli.commands.build.build_context.BuildContext._warn_using_source_maps")
@patch("samcli.commands.build.build_context.BuildContext._warn_invalid_node_options")
def test_warnings_printed(self, warn_node_option_mock, warn_source_map_mock, is_enable_source_map_mock):
build_context = BuildContext(
resource_identifier="resource_id",
template_file="template_file",
base_dir="base_dir",
build_dir="build_dir",
cache_dir="cache_dir",
cached=False,
parallel=False,
mode="mode",
)

stack = DummyStack(
{
"test": {
"Properties": {
"Environment": {"Variables": {"NODE_OPTIONS": ["--something"]}},
},
"Metadata": {"BuildMethod": "esbuild", "BuildProperties": {"Sourcemap": True}},
}
}
)
build_context._stacks = [stack]

is_enable_source_map_mock.return_value = False
build_context._enable_source_maps()

warn_node_option_mock.assert_called()
warn_source_map_mock.assert_called()