Issue 194863003: Don't include source maps in release mode.

Issue 194863003: Don't include source maps in release mode. (Closed)

Created:
6 years, 9 months ago by Bob Nystrom

Modified:
6 years, 9 months ago

Reviewers:
nweiz

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Since release mode doesn't include the Dart sources that the source maps would point to, there's no point in including them in a release build. RELNOTE=Don't include source maps in release builds. R=nweiz@google.com Committed: https://code.google.com/p/dart/source/detail?r=33553

Patch Set 1 #

Patch Set 2 : Don't include source maps at all in release builds. #

Total comments: 4

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+59 lines, -88 lines)			Patch
M	sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart	View	1	6 chunks	+22 lines, -3 lines	2 comments	Download
M	sdk/lib/_internal/pub/lib/src/command/list_package_dirs.dart	View	1	1 chunk	+0 lines, -2 lines	0 comments	Download
M	sdk/lib/_internal/pub/lib/src/dart.dart	View		2 chunks	+6 lines, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_benchmark_test.dart	View	1	1 chunk	+1 line, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_example_test.dart	View	1	1 chunk	+1 line, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_test_test.dart	View	1	1 chunk	+1 line, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_web_test.dart	View	1	1 chunk	+1 line, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/copies_browser_js_next_to_entrypoints_test.dart	View	1	3 chunks	+2 lines, -6 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/copies_browser_js_with_dependency_override_test.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	sdk/lib/_internal/pub/test/build/copies_browser_js_with_dev_dependency_test.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	sdk/lib/_internal/pub/test/build/dart2js_finds_imports_across_packages_test.dart	View	1	1 chunk	+2 lines, -3 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/build/ignores_existing_compiled_js_files_test.dart	View	1	1 chunk	+2 lines, -2 lines	2 comments	Download
M	sdk/lib/_internal/pub/test/build/outputs_results_to_json_test.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
D	sdk/lib/_internal/pub/test/transformer/dart2js/includes_source_map_urls_test.dart	View		1 chunk	+0 lines, -38 lines	0 comments	Download
A +	sdk/lib/_internal/pub/test/transformer/dart2js/includes_source_maps_in_debug_test.dart	View	1	2 chunks	+5 lines, -7 lines	0 comments	Download
A +	sdk/lib/_internal/pub/test/transformer/dart2js/omits_source_map_in_release_test.dart	View	1	2 chunks	+12 lines, -7 lines	0 comments	Download
M	sdk/lib/_internal/pub/test/transformer/dart2js/supports_configuration_with_build_test.dart	View	1	2 chunks	+1 line, -2 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

nweiz

lgtm, although it might also be nice to have a test that serve in release ...

6 years, 9 months ago (2014-03-11 20:10:30 UTC) #3

Bob Nystrom

> although it might also be nice to have a test that serve in release ...

6 years, 9 months ago (2014-03-11 20:43:30 UTC) #4

> although it might also be nice to have a test that serve in release mode
doesn't include source maps.

I considered that, but it felt like overkill to me. I still don't have a clear
guideline for which things should be redundantly tested between pub build and
serve, but this felt like it didn't need to be.

One thing I'm considering is turning the dart2js transformer tests into unit
tests which hit the transformer directly. That would make this a moot point.
But, for now, I think we kind of just pick and choose build or serve for the
tests.

- bob

https://codereview.chromium.org/194863003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/194863003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:97:
generateSourceMaps: _settings.mode != BarbackMode.RELEASE);
On 2014/03/11 20:10:30, nweiz wrote:
> It might be nice to add a "sourceMaps" configuration flag so this can be
> configured independently from the mode (or a TODO to do so).

I'm a little worried that we have too many flags right now, so I'm not inclined
to add another unless we get users specifically asking for it. Since source maps
are strictly a debugging tool, I think it would be weird to configure them in
your pubspec. At the same time, I think using the mode here is a good fit.

https://codereview.chromium.org/194863003/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/build/ignores_existing_compiled_js_files_test.dart
(right):

https://codereview.chromium.org/194863003/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/build/ignores_existing_compiled_js_files_test.dart:35:
output: new RegExp(r'Built 8 files to "build".'));
On 2014/03/11 20:10:30, nweiz wrote:
> Why did this go from 6 to 8 files?

Since this is a debug build now, it also includes the original file.dart and
subfile.dart source files.

Bob Nystrom

6 years, 9 months ago (2014-03-11 20:44:30 UTC) #5

Message was sent while issue was closed.

Committed patchset #2 manually as r33553 (presubmit successful).

Expand Messages | Collapse Messages