Skip to content

feat: Changed Sourcemap default value to false #375

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 4 commits into from
Jul 22, 2022
Merged

feat: Changed Sourcemap default value to false #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
411a983
Changed Sourcemap default to false
lucashuy Jul 14, 2022
5a7acdc
Changed source map DESIGN line to reflect default false change
lucashuy Jul 14, 2022
e799d89
Fixed integ tests for esbuild
lucashuy Jul 15, 2022
ea28e81
Undo change to test exclude sourcemap
lucashuy Jul 15, 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
9 changes: 4 additions & 5 deletions aws_lambda_builders/workflows/nodejs_npm_esbuild/DESIGN.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,24 +117,23 @@ testing flow before invoking `sam build`. For additional typescript caveats with

#### Configuring the bundler

The Lambda builder invokes `esbuild` with sensible defaults that will work for the majority of cases. Importantly, the following three parameters are set by default
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)
Copy link
Contributor

@sriram-mv sriram-mv Jul 20, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lucashuy lucashuy Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sriram-mv sriram-mv Jul 21, 2022

Choose a reason for hiding this comment

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

Ok thanks, so these are closely related and need to be released together.

Copy link
Contributor

@sriram-mv sriram-mv Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

* `--target es2020`, as it allows for javascript features present in Node 14

Users might want to tweak some of these runtime arguments for a specific project, for example not including the source map to further reduce the package size, or restricting javascript features to an older version. The Lambda builder allows this with optional sub-properties of the `aws_sam` configuration property.

* `target`: string, corresponding to a supported [esbuild target](https://esbuild.github.io/api/#target) property
* `minify`: boolean, defaulting to `true`
* `sourcemap`: boolean, defaulting to `true`
* `sourcemap`: boolean, defaulting to `false`

Here is an example that deactivates minification and source maps, and supports JavaScript features compatible with Node.js version 10.
Here is an example that deactivates minification, enables source maps, and supports JavaScript features compatible with Node.js version 10.

```json
{
"entry_points": ["included.ts"],
"target": "node10",
"minify": false,
"sourcemap": false
"sourcemap": true
}
3 changes: 0 additions & 3 deletions aws_lambda_builders/workflows/nodejs_npm_esbuild/esbuild.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,6 @@ def build_default_values(self) -> "EsbuildCommandBuilder":
if "minify" not in self._bundler_config:
args.append("--minify")

if "sourcemap" not in self._bundler_config:
args.append("--sourcemap")

LOG.debug("Using the following default args: %s", str(args))

self._command.extend(args)
Expand Down
42 changes: 31 additions & 11 deletions tests/integration/workflows/nodejs_npm_esbuild/test_nodejs_npm_with_esbuild.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_builds_javascript_project_with_dependencies(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand All @@ -81,7 +81,7 @@ def test_builds_javascript_project_with_multiple_entrypoints(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map", "included2.js", "included2.js.map"}
expected_files = {"included.js", "included2.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand All @@ -101,7 +101,7 @@ def test_builds_typescript_projects(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand Down Expand Up @@ -129,7 +129,7 @@ def test_builds_with_external_esbuild(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand Down Expand Up @@ -165,7 +165,7 @@ def test_bundle_with_implicit_file_types(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js.map", "implicit.js.map", "implicit.js", "included.js"}
expected_files = {"implicit.js", "included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand All @@ -191,7 +191,7 @@ def test_bundles_project_without_dependencies(self, runtime):
executable_search_paths=[binpath],
)

expected_files = {"included.js.map", "included.js"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand Down Expand Up @@ -219,7 +219,7 @@ def test_builds_project_with_remote_dependencies_without_download_dependencies_w
executable_search_paths=[binpath],
)

expected_files = {"included.js.map", "included.js"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand All @@ -240,7 +240,7 @@ def test_builds_project_with_remote_dependencies_with_download_dependencies_and_
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js.map", "included.js"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand Down Expand Up @@ -290,7 +290,7 @@ def test_builds_project_without_combine_dependencies(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js.map", "included.js"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand Down Expand Up @@ -318,7 +318,7 @@ def test_builds_javascript_project_with_external(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)
with open(str(os.path.join(self.artifacts_dir, "included.js"))) as f:
Expand All @@ -343,7 +343,7 @@ def test_builds_javascript_project_with_loader(self, runtime):
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
expected_files = {"included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

Expand All @@ -366,3 +366,23 @@ def test_builds_javascript_project_with_loader(self, runtime):
"\turania: astronomy and astrology"
),
)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",)])
def test_includes_sourcemap_if_requested(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "with-deps-esbuild")

options = {"entry_points": ["included.js"], "sourcemap": True}

self.builder.build(
source_dir,
self.artifacts_dir,
self.scratch_dir,
os.path.join(source_dir, "package.json"),
runtime=runtime,
options=options,
experimental_flags=[EXPERIMENTAL_FLAG_ESBUILD],
)

expected_files = {"included.js", "included.js.map"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)
13 changes: 6 additions & 7 deletions tests/unit/workflows/nodejs_npm_esbuild/test_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,12 @@ def test_raises_error_if_entrypoints_empty_list(self):

def test_packages_javascript_with_minification_and_sourcemap(self):
action = EsbuildBundleAction(
"source", "artifacts", {"entry_points": ["x.js"]}, self.osutils, self.subprocess_esbuild, "package.json"
"source",
"artifacts",
{"entry_points": ["x.js"], "sourcemap": True},
self.osutils,
self.subprocess_esbuild,
"package.json",
)
action.execute()

Expand Down Expand Up @@ -97,7 +102,6 @@ def test_packages_with_externals(self):
"--target=es2020",
"--format=cjs",
"--minify",
"--sourcemap",
"--external:fetch",
"--external:aws-sdk",
],
Expand All @@ -123,7 +127,6 @@ def test_packages_with_custom_loaders(self):
"--target=es2020",
"--format=cjs",
"--minify",
"--sourcemap",
"--loader:.proto=text",
"--loader:.json=js",
],
Expand Down Expand Up @@ -206,7 +209,6 @@ def test_does_not_minify_if_requested(self):
"--outdir=artifacts",
"--target=es2020",
"--format=cjs",
"--sourcemap",
],
cwd="source",
)
Expand All @@ -229,7 +231,6 @@ def test_uses_specified_target(self):
"--outdir=artifacts",
"--format=cjs",
"--minify",
"--sourcemap",
"--target=node14",
],
cwd="source",
Expand All @@ -254,7 +255,6 @@ def test_includes_multiple_entry_points_if_requested(self):
"--outdir=artifacts",
"--format=cjs",
"--minify",
"--sourcemap",
"--target=node14",
],
cwd="source",
Expand Down Expand Up @@ -287,7 +287,6 @@ def test_includes_building_with_external_dependencies(self, osutils_mock):
"--outdir=artifacts",
"--format=cjs",
"--minify",
"--sourcemap",
"--target=node14",
],
cwd="source",
Expand Down
3 changes: 0 additions & 3 deletions tests/unit/workflows/nodejs_npm_esbuild/test_esbuild.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ def test_builds_default_values(self, osutils_mock):
"--target=es2020",
"--format=cjs",
"--minify",
"--sourcemap",
],
)

Expand Down Expand Up @@ -180,7 +179,6 @@ def test_combined_builder_exclude_all_dependencies(self, osutils_mock):
"--target=es2020",
"--format=cjs",
"--minify",
"--sourcemap",
"--loader:.proto=text",
"--loader:.json=js",
"--external:@faker-js/faker",
Expand Down Expand Up @@ -237,7 +235,6 @@ def test_combined_builder_with_dependencies(self, osutils_mock):
"--outdir=artifacts",
"--target=es2020",
"--minify",
"--sourcemap",
"--format=esm",
"--loader:.proto=text",
"--loader:.json=js",
Expand Down