Add improved support for transpiled code by DominicKramer · Pull Request #159 · googleapis/cloud-debug-nodejs

DominicKramer · 2016-10-14T19:13:45Z

A Sourcemapper class was introduced that provides improved support for transpiled code and has been tested with Babel, Coffeescript, and Typescript.

In particular, v8debugapi.js uses a Sourcemapper to understand the connections between a source file and the transpiled code associated with that source file (if that source file has transpiled code associated with it). For this to work, the transpilation process used to generate the transpiled code needs to also generate sourcemap files.

This pull request also fixes bug #153 "Off-by-one-line error in transpiled breakpoint".

If an error occurs when re-registering a debuggee, the name of the debuggee is reported in the error message to aid in debugging why the re-registration failed. fixes googleapis#128

The debugletapi#init() method was updated to accept a callback of the form 'callback(err, project)' instead of 'callback(err)'. The test code for the class was updated so that callbacks now include the 'project' parameter.

When the debugletapi.init() method calls its callback, the project ID reported in the callback should match the project ID stored in the debugletapi object.

The testing of the debugletapi#init() method was updated so that the project name given through the callback was the project name expected.

Now the test for the debugletapi#init() method doesn't stub out the utils.getProjectNumber method because it is already being stubbed out earlier in the code.

The tests in teh test-debugletapi.js file assumed the GCLOUD_PROJECT environment variable was not set, and would fail if this assumption wasn't valid. Now the GCLOUD_PROJECT environment variable is unset when running this file.

…odejs

Two resolve a line in an input file to a line in a transpiled output file, two things needed to be fixed: 1. The line number given to a SourceMapConsumer needs to be one-based. 2. The path given to a SourceMapConsumer needs to be relative to the path of the sourcemap used to build the consumer.

The SourceMapConsumer#generatedPositionFor() seems to return an incorrect line number. sourcemapper.js has been updated to use the SourceMapConsumer#allGeneratedPositionsFor() method that seems to return the correct line number. This has been confirmed using the newly added unit tests that verifies line mappings that were confirmed manually.

The fix consisted of: 1. using the path.resolve() method to resolve a realtive path to its absolute path 2. moving test-sourcemapper.js to the 'test/standalone' directory

The unit test in question is now no longer needed based on the introduction of sourcemapper.js.

DominicKramer · 2016-10-14T19:14:36Z

@GoogleCloudPlatform/node-team PTAL

The unit test in question tested that an error was thrown if the `endsWith` function was supplied `null` or `undefined`. However, this behavior is different on Node v0.12 compared to Node 4, 5, and 6. The unit test to address these differences will be added at a later time.

lib/polyfill.js

@@ -0,0 +1,26 @@
+/**
+ * Copyright 2015 Google Inc. All Rights Reserved.


This commit fixes a bug where after a breakpoint was hit in a source file that was the input to a transpilation process, the breakpoint would be correctly hit, but the breakpoint's line number would incorrectly get changed.

test/test-polyfill.js

+      assert.strictEqual(polyfill.endsWith('some text', 'TEXT'), false);
+      done();
+    });
+});


test/standalone/test-sourcemapper.js

+
+// ---------------------- supplemental information ------------------------
+
+/*


lib/polyfill.js

+    if (typeof String.prototype.endsWith === 'function') {
+      return str.endsWith(suffix);
+    } else {
+      return str.indexOf(suffix, str.length - suffix.length) !== -1;


lib/sourcemapper.js

+ * @param {string} inputPath The absolute path to an input file that could
+ *  possibly be the input to a transpilation process
+ */
+Sourcemapper.prototype.hasMappingInfo = function(inputPath) {


In particular, the lodash#endsWith method will now be used so that polyfill.js is not needed.

A new `test/fixtures/sourcemapper` directory was made to hold the input, output, sourcemap, and build files for the data used in the updated `test-sourcemapper.js` file.

DominicKramer · 2016-10-18T03:40:39Z

@GoogleCloudPlatform/node-team PTAL

This follows the naming convention of private members used in the other files in the project.

lib/sourcemapper.js

@@ -0,0 +1,173 @@
+/**
+ * Copyright 2015 Google Inc. All Rights Reserved.


lib/polyfill.js

@@ -0,0 +1,26 @@
+/**
+ * Copyright 2015 Google Inc. All Rights Reserved.


lib/scanner.js

 * @param {!function(?Error, Array<string>)} callback error-back callback
 */
-function findJSFiles(baseDir, callback) {
+function findFiles(baseDir, regex, callback) {


lib/sourcemapper.js

+          return;
+        }
+
+        try {


lib/sourcemapper.js

+           */
+          var outputBase = consumer.file ? consumer.file
+                                         : path.basename(mapPath, '.map'),
+              parentDir = path.dirname(mapPath),


lib/v8debugapi.js

+      else {
+        var line = breakpoint.location.line,
+            column = 0,
+            mapInfo = sourceMapper.mappingInfo(baseScriptPath, line, column);


In particular, the new Sourcemapper#do() function is the main access point for the Sourcemapper that allows Sourcemapper methods to be safely used even though the Sourcemapper's constructor is asynchronous.

The SourcemapperApi class encapsulates the Sourcemapper's public API and is only exposed through the Sourcemapper#do function. This ensures a Sourcemapper's methods are only used through a Sourcemapper#do method and not directly.

DominicKramer · 2016-10-19T22:35:25Z

@GoogleCloudPlatform/node-team PTAL. There is a stacktrace printed out when the unit tests are run, but the tests still pass. I will look into this, but I wanted to get feedback on the current code.

lib/sourcemapper.js

+  * In particular, the generatedPositionFor() alone doesn't appear to 
+  * give the correct mapping information.
+  */
+  var mappedPos = allPos && allPos.length > 0 ?


lib/sourcemapper.js

+    Array.prototype.reduce.call(allPos,
+      function(accumulator, value, index, arr) {
+          return value.line < accumulator.line ? value : accumulator;
+      }) : consumer.generatedPositionFor(sourcePos);


lib/sourcemapper.js

+ *  processing the given sourcemap files
+ * @constructor
+ */
+function Sourcemapper(sourcemapPaths, logger) {


lib/sourcemapper.js

+ *  processing the given sourcemap files
+ * @constructor
+ */
+function Sourcemapper(sourcemapPaths, logger) {


lib/sourcemapper.js

+    that.api_ = new SourcemapperApi(that.infoMap_);
+
+    _.forEach(that.waitingCallQueue_, function(callback){
+      process.nextTick(function() {


* Renamed `Sourcemapper` to `SourceMapper` * Now the `SourceMapper.create()` method is used to create a `SourceMapper` and the constructed `SourceMapper` is passed to the callback supplied to the `create()` method. * Updated `v8debugapi.create()` to accept a `SourceMapper` * Updated `debuglet.js` to create a SourceMapper

The `scanner.scan()` method now provides a `ScanResults` object to its callback. This new class provides the `selectStats()`, `selectFiles()`, and `all()` methods to eliminate duplication of code that was filtering results obtained from `scanner.scan()`.

DominicKramer · 2016-10-25T21:35:41Z

@GoogleCloudPlatform/node-team PTAL

I've updated the code to only use callbacks. In particular, debuglet.js now uses SourceMapper.create() to create a SourceMapper that is provided through a callback. That SourceMapper is then supplied to v8debugapi.create().

I've also made changes that should address all of the comments made so far.

In particular, the filesystem on my local machine is not case sensitive in this respect and so locally unit tests were passing but they were failing in CI.

lib/sourcemapper.js

+                   };
+                 });
+
+  async.parallel(callList, function(err) {


lib/sourcemapper.js

+  // this handles the case when the path is undefined, null, or
+  // the empty string
+  if (!mapPath || !_.endsWith(mapPath, MAP_EXT)){
+    return callback(new Error('The path ' + mapPath +


In particular, the use of `async.parallel()` was replaced with `async.parallelLimit()` to prevent too many parallel I/O operations from occurring in parallel.

lib/sourcemapper.js

      if (err){
-        return callback(new Error('Could not read sourcemap file ' + mapPath +
-                        ': ' + err));
+        return setImmediate(function() {


lib/sourcemapper.js

      catch(e) {
-        return callback(new Error('An error occurred while reading the '+
-                        'sourcemap file ' + mapPath + ': ' + e));
+        return setImmediate(function() {


lib/sourcemapper.js

      if (sources.length === 0) {
-        return callback(new Error('No sources listed in the sourcemap file ' +
-          mapPath));
+        return setImmediate(function() {


lib/sourcemapper.js

      });

-      callback(null);
+      setImmediate(function() {


ofrobots

LGTM. Please squash before landing.

DominicKramer added 20 commits October 3, 2016 11:55

Add debuggee name to the re-register log message

9996f77

If an error occurs when re-registering a debuggee, the name of the debuggee is reported in the error message to aid in debugging why the re-registration failed. fixes googleapis#128

Update debugletapi test code to use new init() api

bb749c8

The debugletapi#init() method was updated to accept a callback of the form 'callback(err, project)' instead of 'callback(err)'. The test code for the class was updated so that callbacks now include the 'project' parameter.

Add test to validate setting debugletapi.project_

8f8dffb

When the debugletapi.init() method calls its callback, the project ID reported in the callback should match the project ID stored in the debugletapi object.

Add more robust checking of project names

e2fee85

The testing of the debugletapi#init() method was updated so that the project name given through the callback was the project name expected.

Cleaned up the test for the init() method

eadf974

Now the test for the debugletapi#init() method doesn't stub out the utils.getProjectNumber method because it is already being stubbed out earlier in the code.

Fix a bug involving the GCLOUD_PROJECT env var

a0d794e

The tests in teh test-debugletapi.js file assumed the GCLOUD_PROJECT environment variable was not set, and would fail if this assumption wasn't valid. Now the GCLOUD_PROJECT environment variable is unset when running this file.

Now the project is passed to all init cb calls

b8b9653

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-debug-n…

8db4a1a

…odejs

Initial commit of new sourcemap support

0fb93d4

Rename some variables to add clarity

4bab8ea

Add documentation to the sourcemapper file

1646b6d

Remove some now unused error messages

14c9988

[SQUASH] Add unit tests for polyfill.js

49b77e3

[SQUASH] Fix broken v8debugapi unit tests

48e5fc7

The fix consisted of: 1. using the path.resolve() method to resolve a realtive path to its absolute path 2. moving test-sourcemapper.js to the 'test/standalone' directory

[SQUASH] Remove a redundant call to path.normalize

c8a05a1

[SQUASH] Add a .js extension to a proxyquire

a19e651

Merge remote-tracking branch 'upstream/master'

a9c3ba5

[SQUASH] Remove a unit test

00ccd78

The unit test in question is now no longer needed based on the introduction of sourcemapper.js.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2016

Splaktar reviewed Oct 14, 2016

View reviewed changes

Splaktar mentioned this pull request Oct 14, 2016

TypeScript support #160

Closed

[SQUASH] Fix bug in breakpts moving after being hit

329cd2a

This commit fixes a bug where after a breakpoint was hit in a source file that was the input to a transpilation process, the breakpoint would be correctly hit, but the breakpoint's line number would incorrectly get changed.

matthewloring suggested changes Oct 15, 2016

View reviewed changes

DominicKramer added 3 commits October 15, 2016 00:09

[SQUASH] Remove polyfill.js in favor of lodash

5c2cc01

In particular, the lodash#endsWith method will now be used so that polyfill.js is not needed.

[SQUASH] Update Sourcemapper to use relative paths

54cda4f

[SQUASH] Move sourcemap test data under fixtures

62097bf

A new `test/fixtures/sourcemapper` directory was made to hold the input, output, sourcemap, and build files for the data used in the updated `test-sourcemapper.js` file.

[SQUASH] Rename infoMap to infoMap_

1ea49fb

This follows the naming convention of private members used in the other files in the project.

ofrobots suggested changes Oct 18, 2016

View reviewed changes

DominicKramer added 3 commits October 19, 2016 14:14

[SQUASH] Updated to use the new Sourcemapper API

97e7b85

In particular, the new Sourcemapper#do() function is the main access point for the Sourcemapper that allows Sourcemapper methods to be safely used even though the Sourcemapper's constructor is asynchronous.

[SQUASH] Update the docs for scanner#findFiles

8d0794e

[SQUASH] Add docs to the sourcemapper class

d4ce4cb

Splaktar mentioned this pull request Oct 19, 2016

Greatly increased error logging over the last few days #152

Closed

[SQUASH] Create a new SourcemapperApi class

578945f

The SourcemapperApi class encapsulates the Sourcemapper's public API and is only exposed through the Sourcemapper#do function. This ensures a Sourcemapper's methods are only used through a Sourcemapper#do method and not directly.

[SQUASH] Add async as a dependency

9af7559

matthewloring reviewed Oct 24, 2016

View reviewed changes

ofrobots suggested changes Oct 24, 2016

View reviewed changes

DominicKramer added 2 commits October 25, 2016 13:19

[SQUASH] Add ScanResults class

d28cdbe

The `scanner.scan()` method now provides a `ScanResults` object to its callback. This new class provides the `selectStats()`, `selectFiles()`, and `all()` methods to eliminate duplication of code that was filtering results obtained from `scanner.scan()`.

[SQUASH] Fix a capitalization issue in require()

2709b88

In particular, the filesystem on my local machine is not case sensitive in this respect and so locally unit tests were passing but they were failing in CI.

matthewloring approved these changes Oct 25, 2016

View reviewed changes

ofrobots suggested changes Oct 26, 2016

View reviewed changes

DominicKramer added 2 commits October 27, 2016 11:11

[SQUASH] Use limitParallel() in sourcemapper.js

0230d8a

In particular, the use of `async.parallel()` was replaced with `async.parallelLimit()` to prevent too many parallel I/O operations from occurring in parallel.

[SQUASH] Made callbacks in processSourcemaps async

cf40dbc

matthewloring mentioned this pull request Oct 27, 2016

Update to Specifying Configuration When Starting the Cloud Debugger #166

Closed

ofrobots suggested changes Oct 28, 2016

View reviewed changes

[SQUASH] Remove unnecessary setImmediate() calls

775d5b2

ofrobots approved these changes Oct 28, 2016

View reviewed changes

DominicKramer merged commit f8bb4dc into googleapis:master Oct 28, 2016

DominicKramer mentioned this pull request Oct 31, 2016

Off-by-one-line error in transpiled breakpoint #153

Closed

This was referenced Nov 9, 2016

Support transpiled JavaScript #37

Closed

Support for es6 #150

Closed

		@@ -0,0 +1,26 @@
		/**
		* Copyright 2015 Google Inc. All Rights Reserved.


		// ---------------------- supplemental information ------------------------

		/*

		@@ -0,0 +1,173 @@
		/**
		* Copyright 2015 Google Inc. All Rights Reserved.

+                        return;
+                      }
+                      try {

Conversation

DominicKramer commented Oct 14, 2016

Uh oh!

DominicKramer commented Oct 14, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

DominicKramer commented Oct 18, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

DominicKramer commented Oct 19, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

DominicKramer commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

DominicKramer commented Oct 25, 2016 •

edited

Loading