Support generating inlined source maps and wrapping module contents within a JavaScript eval block …

Support generating inlined source maps and wrapping module contents within a JavaScript eval block to improve the debugging experience.

BUG=
R=vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/bda62939a2c4d0fb470ff3b28e07b4ad1bacf41a

[Jacob, 2016-09-13 21:40:39]

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

[Jacob, 2016-09-13 21:40:40]

A side effect of using JavaScript eval is we unfortunately have to specify the
source maps in terms of absolute rather than relative paths.
To make this work we provide a fileMappings argument that maps paths to
appropriate absolute paths on the server. This has the added benefit that paths
to generated files are now correct as the build system has the information
needed to handle them correctly.

[Jennifer Messerly, 2016-09-13 21:55:07]

On 2016/09/13 21:40:40, Jacob wrote:
> A side effect of using JavaScript eval is we unfortunately have to specify the
> source maps in terms of absolute rather than relative paths.
> To make this work we provide a fileMappings argument that maps paths to
> appropriate absolute paths on the server. This has the added benefit that
paths
> to generated files are now correct as the build system has the information
> needed to handle them correctly.

how will this work with Bazel caching? absolute paths on one machine don't
necessary work on others. What if ports change?

This sounds to me like it might need to be a server rewrite. How do other
languages handle this by comparison? e.g. closure compiler, typescript?

[vsm, 2016-09-13 22:08:21]

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:301: ..addOption('file-mapping',
Can you include an example of how this is used (e.g., in a Bazel context)?  Not
clear what "short_path" means in your comment.

[vsm, 2016-09-13 22:17:32]

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:301: ..addOption('file-mapping',
On 2016/09/13 22:08:20, vsm wrote:
> Can you include an example of how this is used (e.g., in a Bazel context)? 
Not
> clear what "short_path" means in your comment.

This might make more sense as --bazel-mapping.  The logic seems very bazel
specific.

[Jennifer Messerly, 2016-09-13 22:43:38]

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

[Jennifer Messerly, 2016-09-13 22:43:39]

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:301: ..addOption('file-mapping',
On 2016/09/13 22:17:32, vsm wrote:
> On 2016/09/13 22:08:20, vsm wrote:
> > Can you include an example of how this is used (e.g., in a Bazel context)? 
> Not
> > clear what "short_path" means in your comment.
> 
> This might make more sense as --bazel-mapping.  The logic seems very bazel
> specific.

yeah ... if some of these options are meant to go together can we merge them
please? We already have unfixed bugs about how awful DDC's command line UX is,
and I had to apologize and make a note to self to be more stringent in code
reviews :)

also if it doesn't make sense for a user to pass it, use "hide: true"

[Jennifer Messerly, 2016-09-13 23:00:33]

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2337213003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:437: if (options.wrapInEval) {
this is not the right place for this. I'm not sure the goal here but this should
part of JS code gen or module transform, not something that gets done by the
"write code" method...

As it is, could you do this in a post process step? it doesn't seem like
something that makes sense for our compiler to being doing.

[vsm, 2016-09-13 23:21:17]

general LGTM to land so we can iterate

Can you please:
(1) Flesh out the comments on --file-mappings?
(2) File an issue to extract eval conversion
(3) File an issue to rationalize all bazel related path flags
(module-root/file-mappings)

[Jacob, 2016-09-14 02:07:35]

changed --file-mappings to --bazel-mapping
to make the intent a bit clearer.

discussed offline with jmesserly and changed --wrap-in-eval
to --single-out-file which is more consistent with other compilers targeting
JavaScript that have a flag to indicate that single output file mode should be
used.
We now check and report an error if --single-out-file is combined with a module
format that does not support it. Currently only the legacy and amd module
formats allow this flag.
Additionally fixed the amd module format to output the module name as part of
the define statement as is the correct behavior in single file output mode. 
Filed https://github.com/dart-lang/sdk/issues/27348
to resolve possibly redundant bazel related flags.

[Jacob, 2016-09-14 02:08:14]

Also got a bit carried away and added sourcemaps for the sdk. Not sure why we
hadn't included them previously.

[Jennifer Messerly, 2016-09-14 15:53:02]

On 2016/09/14 02:08:14, Jacob wrote:
> Also got a bit carried away and added sourcemaps for the sdk. Not sure why we
> hadn't included them previously.

I think the idea there is the SDK code isn't very user-friendly and typically
we'd want to hide those frames/methods from the debugger anyway, unless someone
explicitly enables it.
for example, if someone does "step in" to "obj.method(foo)" it should go to
their method, not our dsend helper. Same thing with type checks, etc.

also is just more bytes to send down.

[Jacob, 2016-09-14 16:48:26]

On 2016/09/14 15:53:02, John Messerly wrote:
> On 2016/09/14 02:08:14, Jacob wrote:
> > Also got a bit carried away and added sourcemaps for the sdk. Not sure why
we
> > hadn't included them previously.
> 
> I think the idea there is the SDK code isn't very user-friendly and typically
> we'd want to hide those frames/methods from the debugger anyway, unless
someone
> explicitly enables it.
> for example, if someone does "step in" to "obj.method(foo)" it should go to
> their method, not our dsend helper. Same thing with type checks, etc.
> 
> also is just more bytes to send down.

1. Source maps are in a separate file for the sdk so it is only ~100 extra bytes
to send down if source maps aren't enabled on the users machine.
2. stepping in to dsend is orthogonal. Users will step in to dsend regardless of
whether there is a source map for that file. To avoid stepping in to dsend that
script needs to be blackboxed. I would expect we would want to tell users to
blackbox just operations.dart which would have the benefit that they can still
step into the Dart Map, Future, Element, or other class where it is helpful.
3. Currently stack frames in the sdk all show up under the same file. Would be
really nice to tell what library a frame is from just looking at the stack
trace.
4. Happy to remove this from the CL. It isn't overly high priority.

[vsm, 2016-09-14 16:52:42]

On 2016/09/14 16:48:26, Jacob wrote:
> On 2016/09/14 15:53:02, John Messerly wrote:
> > On 2016/09/14 02:08:14, Jacob wrote:
> > > Also got a bit carried away and added sourcemaps for the sdk. Not sure why
> we
> > > hadn't included them previously.
> > 
> > I think the idea there is the SDK code isn't very user-friendly and
typically
> > we'd want to hide those frames/methods from the debugger anyway, unless
> someone
> > explicitly enables it.
> > for example, if someone does "step in" to "obj.method(foo)" it should go to
> > their method, not our dsend helper. Same thing with type checks, etc.
> > 
> > also is just more bytes to send down.
> 
> 1. Source maps are in a separate file for the sdk so it is only ~100 extra
bytes
> to send down if source maps aren't enabled on the users machine.
> 2. stepping in to dsend is orthogonal. Users will step in to dsend regardless
of
> whether there is a source map for that file. To avoid stepping in to dsend
that
> script needs to be blackboxed. I would expect we would want to tell users to
> blackbox just operations.dart which would have the benefit that they can still
> step into the Dart Map, Future, Element, or other class where it is helpful.
> 3. Currently stack frames in the sdk all show up under the same file. Would be
> really nice to tell what library a frame is from just looking at the stack
> trace.
> 4. Happy to remove this from the CL. It isn't overly high priority.

Still lgtm

Beyond this CL: I think to handle SDK nicely, we'll want to pull the runtime
helper code out of dart_sdk.js.  Or can we blackbox individual dart files with
source maps?

[Jennifer Messerly, 2016-09-14 16:58:13]

On 2016/09/14 16:52:42, vsm wrote:
> On 2016/09/14 16:48:26, Jacob wrote:
> > On 2016/09/14 15:53:02, John Messerly wrote:
> > > On 2016/09/14 02:08:14, Jacob wrote:
> > > > Also got a bit carried away and added sourcemaps for the sdk. Not sure
why
> > we
> > > > hadn't included them previously.
> > > 
> > > I think the idea there is the SDK code isn't very user-friendly and
> typically
> > > we'd want to hide those frames/methods from the debugger anyway, unless
> > someone
> > > explicitly enables it.
> > > for example, if someone does "step in" to "obj.method(foo)" it should go
to
> > > their method, not our dsend helper. Same thing with type checks, etc.
> > > 
> > > also is just more bytes to send down.
> > 
> > 1. Source maps are in a separate file for the sdk so it is only ~100 extra
> bytes
> > to send down if source maps aren't enabled on the users machine.
> > 2. stepping in to dsend is orthogonal. Users will step in to dsend
regardless
> of
> > whether there is a source map for that file. To avoid stepping in to dsend
> that
> > script needs to be blackboxed. I would expect we would want to tell users to
> > blackbox just operations.dart which would have the benefit that they can
still
> > step into the Dart Map, Future, Element, or other class where it is helpful.
> > 3. Currently stack frames in the sdk all show up under the same file. Would
be
> > really nice to tell what library a frame is from just looking at the stack
> > trace.
> > 4. Happy to remove this from the CL. It isn't overly high priority.
> 
> Still lgtm
> 
> Beyond this CL: I think to handle SDK nicely, we'll want to pull the runtime
> helper code out of dart_sdk.js.  Or can we blackbox individual dart files with
> source maps?

little fyi ... Dart Map, Future, Element are not exactly friendly to step into.
And I'm saying that as someone who's tried :)

- Map in DDC has a lot of low level optimizations (thanks to someone's hard work
in dart2js) ... Dart VM's Map is fairly understandable tho
- Future does almost nothing, all of the logic is deep in dart:async's event
dispatch, I would lay good odds most developers can't understand it
- Element is purely wrapping native C++ code so there's nothing to step into

that's kind of the problem with SDK libraries. Some code like ListMixin that's
pure Dart code is probably nice to go into, but you quickly get into runtime
details with any of dart:core types

[Jacob, 2016-09-14 17:54:52]

Description was changed from

==========
Support generating inlined source maps and wrapping module contents within a
JavaScript eval block to improve the debugging experience.

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

to

==========
Support generating inlined source maps and wrapping module contents within a
JavaScript eval block to improve the debugging experience.

BUG=
R=vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bda62939a2c4d0fb470ff3b28e07b4ad1bacf41a
==========

[Jacob, 2016-09-14 17:54:54]

Committed patchset #4 (id:50001) manually as
bda62939a2c4d0fb470ff3b28e07b4ad1bacf41a (presubmit successful).