Support a new source map bundle format useful for the Dart Dev Compiler. A source map bundle is a J…

Support a new source map bundle format useful for the Dart Dev Compiler. A source map bundle is a JSON array where each entry is a source map.

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/source_maps/commit/453723b89bd49ff9af0dec9a13b58b18b7ebba9d

[Jacob, 2016-12-07 17:05:21]

jacobr@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-12-07 18:06:23]

https://codereview.chromium.org/2560623003/diff/60001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2560623003/diff/60001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Support a new source map bundle format useful for the Dart Dev
Compiler.
minor nits:
 - mention the type that was added, to make it easy to find (e.g. added
`MappingBundle`)
 - mention the actual purpose: to bundle source maps when generating multiple
output files.
 - remove the reference to DDC :)

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart
File lib/parser.dart (right):

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode36
lib/parser.dart:36: Mapping parseJson(/*Map|List*/ json, {Map<String, Map>
otherMaps, mapUrl}) {
Since this is not part of the standard source-map format, I'm thinking we should
split out the MappingBundle piece from here and other APIs below - even though
it has a similar API like `Mapping` it seems like the use case is different
enough to keep them separate. For example, you probably never need to provide
the uri when you have a SingleMapping, but you always need it for the
MappingBundle.

My current inclination is to:
 - keep `parseJson` as before.
 - move the new mapping to a new library (e.g lib/bundle.dart)
 - maybe add a parseJsonBundle(List json, ...) as a separate top-level function
on that file for convenience.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode62
lib/parser.dart:62: /// [uri] is the optional location of the output file to
find the span for
Assuming we agree with splitting the APIs, I'd remove the parameter here.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode166
lib/parser.dart:166: var mapping = new SingleMapping.fromJson(map, mapUrl:
mapUrl);
maybe use `parseJson` so we get the validation we have (version, check for
Single or MultiSection mappings)

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode184
lib/parser.dart:184: {Map<String, SourceFile> files, String uri}) {
2 suggestions:
 - make uri mandatory
 - rename it to outputUri

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode185
lib/parser.dart:185: if (_mappings.containsKey(uri)) {
check that `uri` is not null?

https://codereview.chromium.org/2560623003/diff/60001/test/parser_test.dart
File test/parser_test.dart (right):

https://codereview.chromium.org/2560623003/diff/60001/test/parser_test.dart#n...
test/parser_test.dart:148: group('parse with bundle', () {
assuming we create bundle.dart, I'd move this test group into test_bundle.dart
as well.

[Jacob, 2016-12-07 20:40:32]

https://codereview.chromium.org/2560623003/diff/60001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2560623003/diff/60001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Support a new source map bundle format useful for the Dart Dev
Compiler.
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> minor nits:
>  - mention the type that was added, to make it easy to find (e.g. added
> `MappingBundle`)
>  - mention the actual purpose: to bundle source maps when generating multiple
> output files.
>  - remove the reference to DDC :)

Done.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart
File lib/parser.dart (right):

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode36
lib/parser.dart:36: Mapping parseJson(/*Map|List*/ json, {Map<String, Map>
otherMaps, mapUrl}) {
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> Since this is not part of the standard source-map format, I'm thinking we
should
> split out the MappingBundle piece from here and other APIs below - even though
> it has a similar API like `Mapping` it seems like the use case is different
> enough to keep them separate. For example, you probably never need to provide
> the uri when you have a SingleMapping, but you always need it for the
> MappingBundle.
> 
> My current inclination is to:
>  - keep `parseJson` as before.
>  - move the new mapping to a new library (e.g lib/bundle.dart)
>  - maybe add a parseJsonBundle(List json, ...) as a separate top-level
function
> on that file for convenience.
added parseJsonExtended method
that parses the extended format that allows regular source map format as well as
the bundle format needed to handle multiple output files.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode62
lib/parser.dart:62: /// [uri] is the optional location of the output file to
find the span for
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> Assuming we agree with splitting the APIs, I'd remove the parameter here.

discussed offline. kept but checked.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode70
lib/parser.dart:70: {Map<String, SourceFile> files, String uri}) {
removed uri from this method as
SourceLocation specifies a uri

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode166
lib/parser.dart:166: var mapping = new SingleMapping.fromJson(map, mapUrl:
mapUrl);
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> maybe use `parseJson` so we get the validation we have (version, check for
> Single or MultiSection mappings)

Done. Note we cast to SingleMapping as only SingleMapping has the targetURL
field we depend on.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode184
lib/parser.dart:184: {Map<String, SourceFile> files, String uri}) {
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> 2 suggestions:
>  - make uri mandatory
>  - rename it to outputUri

discussed offline. added check that it is not null instead.

https://codereview.chromium.org/2560623003/diff/60001/lib/parser.dart#newcode185
lib/parser.dart:185: if (_mappings.containsKey(uri)) {
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> check that `uri` is not null?
> 

Done.

https://codereview.chromium.org/2560623003/diff/60001/test/parser_test.dart
File test/parser_test.dart (right):

https://codereview.chromium.org/2560623003/diff/60001/test/parser_test.dart#n...
test/parser_test.dart:148: group('parse with bundle', () {
On 2016/12/07 18:06:22, Siggi Cherem (dart-lang) wrote:
> assuming we create bundle.dart, I'd move this test group into test_bundle.dart
> as well.

Acknowledged.

[Siggi Cherem (dart-lang), 2016-12-07 23:27:00]

small nits below, but otherwise lgtm

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Add `MappingBundle` class that handles extended source map
format that supports
nit: 80 col

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md#newcode5
CHANGELOG.md:5: Extend `Mapper` `spanFor` API to accept a uri parameter that is
optional for
nit: add a bullet for this point too
typo: Mapper => Mapping, and combine it with spanFor: `Mapping.spanFor`

https://codereview.chromium.org/2560623003/diff/80001/lib/parser.dart
File lib/parser.dart (right):

https://codereview.chromium.org/2560623003/diff/80001/lib/parser.dart#newcode80
lib/parser.dart:80: /// [uri] is the optional location of the output file to
find the span for
dartdoc nit: add an empty line above this one.

https://codereview.chromium.org/2560623003/diff/80001/test/parser_test.dart
File test/parser_test.dart (right):

https://codereview.chromium.org/2560623003/diff/80001/test/parser_test.dart#n...
test/parser_test.dart:181: expect(mapping.spanFor(0, 0, uri:
"wrong_output.dart"), isNull);
let's include a test with 'null' :)

[Jacob, 2016-12-08 16:20:22]

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md#newcode3
CHANGELOG.md:3: * Add `MappingBundle` class that handles extended source map
format that supports
On 2016/12/07 23:27:00, Siggi Cherem (dart-lang) wrote:
> nit: 80 col

Done.

https://codereview.chromium.org/2560623003/diff/80001/CHANGELOG.md#newcode5
CHANGELOG.md:5: Extend `Mapper` `spanFor` API to accept a uri parameter that is
optional for
On 2016/12/07 23:27:00, Siggi Cherem (dart-lang) wrote:
> nit: add a bullet for this point too
> typo: Mapper => Mapping, and combine it with spanFor: `Mapping.spanFor`

Done.

https://codereview.chromium.org/2560623003/diff/80001/lib/parser.dart
File lib/parser.dart (right):

https://codereview.chromium.org/2560623003/diff/80001/lib/parser.dart#newcode80
lib/parser.dart:80: /// [uri] is the optional location of the output file to
find the span for
On 2016/12/07 23:27:00, Siggi Cherem (dart-lang) wrote:
> dartdoc nit: add an empty line above this one.

Done.

https://codereview.chromium.org/2560623003/diff/80001/test/parser_test.dart
File test/parser_test.dart (right):

https://codereview.chromium.org/2560623003/diff/80001/test/parser_test.dart#n...
test/parser_test.dart:181: expect(mapping.spanFor(0, 0, uri:
"wrong_output.dart"), isNull);
On 2016/12/07 23:27:00, Siggi Cherem (dart-lang) wrote:
> let's include a test with 'null' :)

Done.

[Jacob, 2016-12-08 16:20:50]

Description was changed from

==========
Support a new source map bundle format useful for the Dart Dev Compiler. A
source map bundle is a JSON array where each entry is a source map.

BUG=
==========

to

==========
Support a new source map bundle format useful for the Dart Dev Compiler. A
source map bundle is a JSON array where each entry is a source map.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/source_maps/commit/453723b89bd49ff9af0dec9a13b58...
==========

[Jacob, 2016-12-08 16:20:51]

Committed patchset #6 (id:100001) manually as
453723b89bd49ff9af0dec9a13b58b18b7ebba9d (presubmit successful).

[nweiz, 2016-12-12 21:57:01]

nweiz@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2016-12-12 21:57:01]

This release violates semantic versioning. It very clearly adds a new feature,
which means it should have had its version bumped to 0.10.2.

Please publish a 0.10.2 release as soon as possible so that people who naively
use a "^" constraint for this package and use its new features don't run into
versioning problems.

Also, both of you please read and internalize the documentation on package
versioning
(https://github.com/dart-lang/site-www/blob/7dcaa9f933c2eda1bc4b9f02e19c9e4bf2...),
and make sure to apply it to future CLs and code reviews. Keeping versioning
semantic is extremely important to maintaining a healthy and usable package
ecosystem.