Merge pull request #11 from parcel-bundler/fix-napi

DeMoorJasper · web-flow · commit 78668c9ac7b4 · 2020-04-04T12:39:17.000+02:00
Making addIndexedMappings more stable
diff --git a/bench/run.js b/bench/run.js
@@ -1,4 +1,5 @@
 const Benchmark = require("tiny-benchy");
+const MozillaSourceMap = require("source-map");
 const assert = require("assert");
 const { default: SourceMap, init } =
   process.env.BACKEND === "wasm" ? require("../dist/wasm-node") : require("../");
@@ -14,12 +15,12 @@ init.then(() => {
       name: "A",
       original: {
         line: index + 1,
-        column: 0 + 10 * index
+        column: 0 + 10 * index,
       },
       generated: {
         line: 1,
-        column: 15 + 10 * index
-      }
+        column: 15 + 10 * index,
+      },
     };
   });
 
@@ -47,6 +48,21 @@ init.then(() => {
     map.addIndexedMappings(mappings);
   });
 
+  suite.add("JS Mappings => vlq (mozilla source-map) => buffer", async () => {
+    let map = new MozillaSourceMap.SourceMapGenerator({
+      sourceRoot: "/",
+    });
+
+    for (let mapping of mappings) {
+      map.addMapping(mapping);
+    }
+
+    let json = map.toJSON();
+
+    let sourceMap = new SourceMap();
+    sourceMap.addRawMappings(json.mappings, json.sources, json.names);
+  });
+
   suite.add("Save buffer", async () => {
     sourcemapInstance.toBuffer();
   });
@@ -60,7 +76,7 @@ init.then(() => {
   suite.add("stringify", async () => {
     await sourcemapInstance.stringify({
       file: "index.js.map",
-      sourceRoot: "/"
+      sourceRoot: "/",
     });
   });
 
@@ -90,7 +106,7 @@ init.then(() => {
     }
     await map.stringify({
       file: "index.js.map",
-      sourceRoot: "/"
+      sourceRoot: "/",
     });
   });
 
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@parcel/source-map",
-  "version": "2.0.0-alpha.4.6",
+  "version": "2.0.0-alpha.4.7",
   "main": "./dist/node.js",
   "browser": "./dist/wasm-browser.js",
   "license": "MIT",
@@ -41,8 +41,7 @@
   },
   "dependencies": {
     "node-addon-api": "^2.0.0",
-    "node-gyp-build": "^4.2.1",
-    "prettier": "^2.0.2"
+    "node-gyp-build": "^4.2.1"
   },
   "devDependencies": {
     "@babel/cli": "^7.8.4",
@@ -54,6 +53,8 @@
     "flow-copy-source": "^2.0.9",
     "mocha": "^7.1.0",
     "prebuildify": "^3.0.4",
-    "tiny-benchy": "^1.0.0"
+    "source-map": "^0.7.3",
+    "tiny-benchy": "^1.0.0",
+    "prettier": "^2.0.2"
   }
 }
diff --git a/src/MappingContainer.cpp b/src/MappingContainer.cpp
@@ -437,3 +437,22 @@ void MappingContainer::addBufferMappings(const void *buf, int lineOffset, int co
         }
     }
 }
+
+void MappingContainer::addIndexedMapping(int generatedLine, int generatedColumn, int originalLine, int originalColumn,
+                                         std::string source, std::string name) {
+    Position generatedPosition = Position{generatedLine, generatedColumn};
+    Position originalPosition = Position{originalLine, originalColumn};
+    int sourceIndex = -1;
+    int nameIndex = -1;
+    if (originalPosition.line > -1) {
+        if (!source.empty()) {
+            sourceIndex = addSource(source);
+        }
+
+        if (!name.empty()) {
+            nameIndex = addName(name);
+        }
+    }
+
+    addMapping(generatedPosition, originalPosition, sourceIndex, nameIndex);
+}
diff --git a/src/MappingContainer.h b/src/MappingContainer.h
@@ -50,6 +50,8 @@ class MappingContainer {
 
     void addBufferMappings(const void *buf, int lineOffset = 0, int columnOffset = 0);
 
+    void addIndexedMapping(int generatedLine, int generatedColumn, int originalLine, int originalColumn, std::string source, std::string name);
+
 private:
     // Processed mappings, for all kinds of modifying within the sourcemap
     std::vector<std::string> _sources;
diff --git a/src/napi/SourceMap.cpp b/src/napi/SourceMap.cpp
@@ -65,12 +65,14 @@ void SourceMapBinding::addBufferMappings(const Napi::CallbackInfo &info) {
     }
 
     if (info.Length() > 1 && !info[1].IsNumber()) {
-        Napi::TypeError::New(env, "Expected a lineOffset of type integer for the second parameter").ThrowAsJavaScriptException();
+        Napi::TypeError::New(env,
+                             "Expected a lineOffset of type integer for the second parameter").ThrowAsJavaScriptException();
         return;
     }
 
     if (info.Length() > 2 && !info[2].IsNumber()) {
-        Napi::TypeError::New(env, "Expected a columnOffset of type integer for the third parameter").ThrowAsJavaScriptException();
+        Napi::TypeError::New(env,
+                             "Expected a columnOffset of type integer for the third parameter").ThrowAsJavaScriptException();
         return;
     }
 
@@ -205,68 +207,24 @@ Napi::Value SourceMapBinding::getMap(const Napi::CallbackInfo &info) {
     return obj;
 }
 
-// addIndexedMappings(array<mapping>, lineOffset, columnOffset): uses numbers for source and name with the index specified in the sources/names map/array in SourceMap instance
-void SourceMapBinding::addIndexedMappings(const Napi::CallbackInfo &info) {
+// addIndexedMapping(generatedLine, generatedColumn, originalLine, originalColumn, source, name)
+void SourceMapBinding::addIndexedMapping(const Napi::CallbackInfo &info) {
     Napi::Env env = info.Env();
     Napi::HandleScope scope(env);
 
-    if (info.Length() < 1) {
-        Napi::TypeError::New(env, "Expected 1-3 parameters").ThrowAsJavaScriptException();
-        return;
-    }
-
-    if (!info[0].IsArray()) {
-        Napi::TypeError::New(env, "First parameter should be an array").ThrowAsJavaScriptException();
-        return;
-    }
-
-    if (info.Length() > 1 && !info[1].IsNumber()) {
-        Napi::TypeError::New(env,
-                             "Second parameter should be a lineOffset of type integer").ThrowAsJavaScriptException();
-        return;
-    }
-
-    if (info.Length() > 2 && !info[2].IsNumber()) {
-        Napi::TypeError::New(env,
-                             "Third parameter should be a lineOffset of type integer").ThrowAsJavaScriptException();
+    if (info.Length() < 6) {
+        Napi::TypeError::New(env, "Expected 6 parameters").ThrowAsJavaScriptException();
         return;
     }
 
-    const Napi::Array mappingsArray = info[0].As<Napi::Array>();
-    int lineOffset = info.Length() > 1 ? info[1].As<Napi::Number>().Int32Value() : 0;
-    int columnOffset = info.Length() > 2 ? info[2].As<Napi::Number>().Int32Value() : 0;
+    int generatedLine = info[0].As<Napi::Number>().Int32Value();
+    int generatedColumn = info[1].As<Napi::Number>().Int32Value();
+    int originalLine = info[2].As<Napi::Number>().Int32Value();
+    int originalColumn = info[3].As<Napi::Number>().Int32Value();
+    std::string source = info[4].As<Napi::String>().Utf8Value();
+    std::string name = info[5].As<Napi::String>().Utf8Value();
 
-    unsigned int length = mappingsArray.Length();
-    for (unsigned int i = 0; i < length; ++i) {
-        Napi::Value mapping = mappingsArray.Get(i);
-        Napi::Object mappingObject = mapping.As<Napi::Object>();
-
-        Napi::Object generated = mappingObject.Get("generated").As<Napi::Object>();
-        int generatedLine = generated.Get("line").As<Napi::Number>().Int32Value() - 1;
-        int generatedColumn = generated.Get("column").As<Napi::Number>().Int32Value();
-        Position generatedPosition = Position{generatedLine + lineOffset, generatedColumn + columnOffset};
-
-        Napi::Value originalPositionValue = mappingObject.Get("original");
-        if (originalPositionValue.IsObject()) {
-            Napi::Object originalPositionObject = originalPositionValue.As<Napi::Object>();
-            int originalLine = originalPositionObject.Get("line").As<Napi::Number>().Int32Value() - 1;
-            int originalColumn = originalPositionObject.Get("column").As<Napi::Number>().Int32Value();
-            Position originalPosition = Position{originalLine, originalColumn};
-
-            std::string sourceString = mappingObject.Get("source").As<Napi::String>().Utf8Value();
-            int source = _mapping_container.addSource(sourceString);
-
-            Napi::Value nameValue = mappingObject.Get("name");
-            if (nameValue.IsString()) {
-                std::string nameString = nameValue.As<Napi::String>().Utf8Value();
-                _mapping_container.addMapping(generatedPosition, originalPosition, source, _mapping_container.addName(nameString));
-            } else {
-                _mapping_container.addMapping(generatedPosition, originalPosition, source);
-            }
-        } else {
-            _mapping_container.addMapping(generatedPosition);
-        }
-    }
+    _mapping_container.addIndexedMapping(generatedLine, generatedColumn, originalLine, originalColumn, source, name);
 }
 
 Napi::Value SourceMapBinding::getSourceIndex(const Napi::CallbackInfo &info) {
@@ -368,7 +326,8 @@ void SourceMapBinding::addEmptyMap(const Napi::CallbackInfo &info) {
     }
 
     if (info.Length() == 3 && !info[2].IsNumber()) {
-        Napi::TypeError::New(env, "Expected third parameter to be a lineOffset of type Integer").ThrowAsJavaScriptException();
+        Napi::TypeError::New(env,
+                             "Expected third parameter to be a lineOffset of type Integer").ThrowAsJavaScriptException();
         return;
     }
 
@@ -387,8 +346,9 @@ Napi::Value SourceMapBinding::findClosestMapping(const Napi::CallbackInfo &info)
         Napi::TypeError::New(env, "Expected 1 parameter of type buffer").ThrowAsJavaScriptException();
         return env.Null();
     }
-    
-    Mapping m = _mapping_container.findClosestMapping(info[0].As<Napi::Number>().Int32Value() - 1, info[1].As<Napi::Number>().Int32Value());
+
+    Mapping m = _mapping_container.findClosestMapping(info[0].As<Napi::Number>().Int32Value() - 1,
+                                                      info[1].As<Napi::Number>().Int32Value());
     return _mappingToObject(env, m);
 }
 
@@ -401,7 +361,7 @@ Napi::Object SourceMapBinding::Init(Napi::Env env, Napi::Object exports) {
             InstanceMethod("stringify", &SourceMapBinding::stringify),
             InstanceMethod("toBuffer", &SourceMapBinding::toBuffer),
             InstanceMethod("getMap", &SourceMapBinding::getMap),
-            InstanceMethod("addIndexedMappings", &SourceMapBinding::addIndexedMappings),
+            InstanceMethod("addIndexedMapping", &SourceMapBinding::addIndexedMapping),
             InstanceMethod("addNames", &SourceMapBinding::addNames),
             InstanceMethod("addSources", &SourceMapBinding::addSources),
             InstanceMethod("getSourceIndex", &SourceMapBinding::getSourceIndex),
diff --git a/src/napi/SourceMap.h b/src/napi/SourceMap.h
@@ -15,6 +15,8 @@ class SourceMapBinding : public Napi::ObjectWrap<SourceMapBinding> {
 
     void addBufferMappings(const Napi::CallbackInfo &info);
 
+    void addIndexedMapping(const Napi::CallbackInfo &info);
+
     void addIndexedMappings(const Napi::CallbackInfo &info);
 
     void extends(const Napi::CallbackInfo &info);
diff --git a/src/node.js b/src/node.js
@@ -65,15 +65,29 @@ export default class SourceMap {
 
   // line numbers start at 1 so we have the same api as `source-map` by mozilla
   addIndexedMappings(
-    mappings: Array<IndexedMapping<number | string>>,
+    mappings: Array<IndexedMapping<string>>,
     lineOffset?: number = 0,
     columnOffset?: number = 0
   ) {
-    this.sourceMapInstance.addIndexedMappings(
-      mappings,
-      lineOffset,
-      columnOffset
-    );
+    for (let mapping of mappings) {
+      let hasValidOriginal =
+        mapping.original &&
+        typeof mapping.original.line === "number" &&
+        !isNaN(mapping.original.line) &&
+        typeof mapping.original.column === "number" &&
+        !isNaN(mapping.original.column);
+
+      this.sourceMapInstance.addIndexedMapping(
+        mapping.generated.line + lineOffset - 1,
+        mapping.generated.column + columnOffset,
+        // $FlowFixMe
+        hasValidOriginal ? mapping.original.line - 1 : -1,
+        // $FlowFixMe
+        hasValidOriginal ? mapping.original.column : -1,
+        mapping.source || "",
+        mapping.name || ""
+      );
+    }
     return this;
   }
 
diff --git a/src/wasm.js b/src/wasm.js
@@ -100,20 +100,25 @@ export default class SourceMap {
     lineOffset?: number = 0,
     columnOffset?: number = 0
   ) {
-    let mappingsVector = new Module.VectorIndexedMapping();
-    for (let m of mappings) {
-      mappingsVector.push_back({
-        generated: m.generated,
-        original: m.original || { line: -1, column: -1 },
-        source: m.source != null ? m.source : "",
-        name: m.name != null ? m.name : "",
-      });
+    for (let mapping of mappings) {
+      let hasValidOriginal =
+        mapping.original &&
+        typeof mapping.original.line === "number" &&
+        !isNaN(mapping.original.line) &&
+        typeof mapping.original.column === "number" &&
+        !isNaN(mapping.original.column);
+
+      this.sourceMapInstance.addIndexedMapping(
+        mapping.generated.line + lineOffset - 1,
+        mapping.generated.column + columnOffset,
+        // $FlowFixMe
+        hasValidOriginal ? mapping.original.line - 1 : -1,
+        // $FlowFixMe
+        hasValidOriginal ? mapping.original.column : -1,
+        mapping.source || "",
+        mapping.name || ""
+      );
     }
-    this.sourceMapInstance.addIndexedMappings(
-      mappingsVector,
-      lineOffset,
-      columnOffset
-    );
     return this;
   }
 
diff --git a/src/wasm/SourceMap.cpp b/src/wasm/SourceMap.cpp
@@ -59,32 +59,9 @@ std::vector<Mapping> SourceMap::getMappings(){
     return mappings;
 }
 
-// addIndexedMappings(array<mapping>, lineOffset, columnOffset): uses numbers for source and name with the index specified in the sources/names map/array in SourceMap instance
-void SourceMap::addIndexedMappings(std::vector<IndexedMapping> mappingsArray, int lineOffset, int columnOffset) {
-    for (auto mapping = mappingsArray.begin(), lineEnd = mappingsArray.end();
-            mapping != lineEnd; ++mapping) {
-
-
-        int generatedLine = mapping->generated.line - 1;
-        int generatedColumn = mapping->generated.column;
-        Position generatedPosition = Position{generatedLine + lineOffset, generatedColumn + columnOffset};
-
-        int originalColumn = mapping->original.column;
-        int originalLine = mapping->original.line - 1;
-        if (originalColumn >= 0 && originalLine >= 0) {
-            Position originalPosition = Position{originalLine, originalColumn};
-
-            int source = _mapping_container.addSource(mapping->source);
-
-            if (mapping->name.size() > 0) {
-                _mapping_container.addMapping(generatedPosition, originalPosition, source, _mapping_container.addName(mapping->name));
-            } else {
-                _mapping_container.addMapping(generatedPosition, originalPosition, source);
-            }
-        } else {
-            _mapping_container.addMapping(generatedPosition);
-        }
-    }
+// addIndexedMapping(generatedLine, generatedColumn, originalLine, originalColumn, source, name)
+void SourceMap::addIndexedMapping(int generatedLine, int generatedColumn, int originalLine, int originalColumn, std::string source, std::string name) {
+    _mapping_container.addIndexedMapping(generatedLine, generatedColumn, originalLine, originalColumn, source, name);
 }
 
 int SourceMap::getSourceIndex(std::string source) {
@@ -131,7 +108,7 @@ EMSCRIPTEN_BINDINGS(my_class_example) {
         .constructor<>()
         .function("addRawMappings", &SourceMap::addRawMappings)
         .function("addBufferMappings", &SourceMap::addBufferMappings)
-        .function("addIndexedMappings", &SourceMap::addIndexedMappings)
+        .function("addIndexedMapping", &SourceMap::addIndexedMapping)
         .function("getVLQMappings", &SourceMap::getVLQMappings)
         .function("getMappings", &SourceMap::getMappings)
         .function("getSources", &SourceMap::getSources)
diff --git a/src/wasm/SourceMap.h b/src/wasm/SourceMap.h
@@ -9,7 +9,7 @@ class SourceMap {
 
     void addRawMappings(std::string rawMappings, std::vector<std::string> sources, std::vector<std::string> names, int lineOffset, int columnOffset);
     void addBufferMappings(std::string mapbuffer, int lineOffset, int columnOffset);
-    void addIndexedMappings(std::vector<IndexedMapping> mappingsArray, int lineOffset, int columnOffset);
+    void addIndexedMapping(int generatedLine, int generatedColumn, int originalLine, int originalColumn, std::string source, std::string name);
 
     void extends(std::string mapBuffer);
 
diff --git a/test/append.test.js b/test/append.test.js
diff --git a/yarn.lock b/yarn.lock
