Merge pull request #153 from EMH333/preprocess-warning-location

Correctly provide file locations for errors and warnings when using preprocessors

CHANGELOG.md

@@ -2,16 +2,20 @@
 
 ## Unreleased (Breaking)
 
-- Update development esbuild version to `0.19.0`
-
-  This means that this plugin now supports additional inhancements when using the `context` esbuild `v0.17.0` API as detailed below.
-
 - **Minorly Breaking** Caching is automatically enabled after two sucessful builds when using the `context` esbuild API
 
   Previously caching was automatically enabled when using the watch or incremental esbuild options, but those were removed in esbuild `v0.17.0`. This change brings back the automatic cache enabling when using the `context` API which supports the same features as the previous watch and incremental options. esbuild does not provide a way for plugins to determine if the `context` API is being used, so this feature is enabled after two successful builds. This should be a reasonable compromise between not enabling the cache at all and enabling it for every build (which wastes time and space if caching isn't needed).
 
   If you are using the `context` API and want to disable the cache, you can set the `cache` option to `false` in the plugin options but this isn't recommended (if you do need to disable the cache for some reason, please open an issue to see if your usecase can be fixed).
 
+- **Minorly Breaking** Add dependency to `@jridgewell/trace-mapping` so error messages are more accurate when using preprocessors ([#83](https://github.com/EMH333/esbuild-svelte/issues/83))
+
+  If you are using Svelte 4, this doesn't add additional dependencies because that package is already required by Svelte 4.
+
+- Update development esbuild version to `0.19.0`
+
+  This means that this plugin now supports additional inhancements when using the `context` esbuild `v0.17.0` API as detailed below.
+
 ## 0.7.4
 
 - Lock Svelte peerDependency to `>=3.43.0 <5` to protect against breaking changes in future Svelte releases

index.ts

@@ -3,10 +3,11 @@ import { preprocess, compile, VERSION } from "svelte/compiler";
 import { dirname, basename, relative } from "path";
 import { promisify } from "util";
 import { readFile, statSync } from "fs";
+import { originalPositionFor, TraceMap } from "@jridgewell/trace-mapping";
 
 import type { CompileOptions, Warning } from "svelte/types/compiler/interfaces";
 import type { PreprocessorGroup } from "svelte/types/compiler/preprocess";
-import type { OnLoadResult, Plugin, PluginBuild } from "esbuild";
+import type { OnLoadResult, Plugin, PluginBuild, Location, PartialMessage } from "esbuild";
 
 interface esbuildSvelteOptions {
     /**
@@ -52,17 +53,40 @@ interface CacheData {
     dependencies: Map<string, Date>;
 }
 
-const convertMessage = ({ message, start, end, filename, frame }: Warning) => ({
-    text: message,
-    location: start &&
-        end && {
+async function convertMessage(
+    { message, start, end }: Warning,
+    filename: string,
+    source: string,
+    sourcemap: any,
+): Promise<PartialMessage> {
+    let location: Partial<Location> | undefined;
+    if (start && end) {
+        let lineText = source.split(/\r\n|\r|\n/g)[start.line - 1];
+        let lineEnd = start.line === end.line ? end.column : lineText.length;
+
+        // Adjust the start and end positions based on what the preprocessors did so the positions are correct
+        if (sourcemap) {
+            sourcemap = new TraceMap(sourcemap);
+            const pos = originalPositionFor(sourcemap, {
+                line: start.line,
+                column: start.column,
+            });
+            if (pos.source) {
+                start.line = pos.line ?? start.line;
+                start.column = pos.column ?? start.column;
+            }
+        }
+
+        location = {
             file: filename,
             line: start.line,
             column: start.column,
-            length: start.line === end.line ? end.column - start.column : 0,
-            lineText: frame,
-        },
-});
+            length: lineEnd - start.column,
+            lineText,
+        };
+    }
+    return { text: message, location };
+}
 
 //still support old incremental option if possible, but can still be overriden by cache option
 const shouldCache = (
@@ -269,7 +293,17 @@ export default function sveltePlugin(options?: esbuildSvelteOptions): Plugin {
 
                     const result: OnLoadResult = {
                         contents,
-                        warnings: warnings.map(convertMessage),
+                        warnings: await Promise.all(
+                            warnings.map(
+                                async (e) =>
+                                    await convertMessage(
+                                        e,
+                                        args.path,
+                                        source,
+                                        compilerOptions.sourcemap,
+                                    ),
+                            ),
+                        ),
                     };
 
                     // if we are told to cache, then cache
@@ -289,7 +323,14 @@ export default function sveltePlugin(options?: esbuildSvelteOptions): Plugin {
                     return result;
                 } catch (e: any) {
                     let result: OnLoadResult = {};
-                    result.errors = [convertMessage(e)];
+                    result.errors = [
+                        await convertMessage(
+                            e,
+                            args.path,
+                            originalSource,
+                            compilerOptions.sourcemap,
+                        ),
+                    ];
                     // only provide if context API is supported or we are caching
                     if (build.esbuild?.context !== undefined || shouldCache(build)) {
                         result.watchFiles = previousWatchFiles;

package.json

@@ -49,5 +49,8 @@
   },
   "engines": {
     "node": ">=14"
+  },
+  "dependencies": {
+    "@jridgewell/trace-mapping": "^0.3.18"
   }
 }

test/errors.mjs

@@ -0,0 +1,47 @@
+import { test } from "uvu";
+import * as assert from "uvu/assert";
+import { build as _build } from "esbuild";
+import { typescript } from "svelte-preprocess-esbuild";
+import sveltePlugin from "../dist/index.mjs";
+
+test("Errors (with preprocessors) are in the right spot", async () => {
+    try {
+        await _build({
+            entryPoints: ["./test/fixtures/errors/entry.js"],
+            outdir: "../example/dist",
+            bundle: true,
+            write: false, //Don't write anywhere
+            sourcemap: true,
+            plugins: [
+                sveltePlugin({
+                    preprocess: [typescript()],
+                    compilerOptions: { dev: true },
+                }),
+            ],
+            logLevel: "silent",
+        });
+    } catch (e) {
+        assert.equal(e.errors.length, 1, "Should have one error");
+        assert.equal(e.warnings.length, 0, "Should not have warnings");
+
+        const error = e.errors[0];
+
+        assert.equal(
+            error.location.file,
+            "test/fixtures/errors/error.svelte",
+            "Should have the right file",
+        );
+        assert.equal(error.location.line, 12, "Should have the right line");
+        assert.equal(error.location.column, 31, "Should have the right column");
+        assert.equal(
+            error.text,
+            "Expected value for the attribute",
+            "Should have the right error message",
+        );
+        return;
+    }
+
+    assert.unreachable("Should have thrown an error");
+});
+
+test.run();

test/fixtures/errors/entry.js

@@ -0,0 +1,5 @@
+import Test from "./error.svelte";
+
+new Test({
+    target: document.body,
+});

test/fixtures/errors/error.svelte

@@ -0,0 +1,12 @@
+<script lang="ts">
+  interface Test {
+    yellThisThing: String;
+  }
+
+  // some sort of javascript error
+  function event() {
+    console.log({ yellThisThing: "something" } as Test);
+  }
+</script>
+
+<button on:click={event} class=>Yell "something"</button>

test/fixtures/errors/svelte.config.js

@@ -0,0 +1,6 @@
+const { typescript } = require("svelte-preprocess-esbuild");
+const { sass } = require("svelte-preprocess-sass");
+
+module.exports = {
+    preprocess: [{ style: sass() }, typescript()],
+};

test/fixtures/preprocessing-sourcemaps/pp-sourcemaps.svelte

@@ -1,33 +1,35 @@
 <script lang="typescript">
-    interface Test {
-        yellThisThing: String;
-    }
+  interface Test {
+    yellThisThing: String;
+  }
 
-    function event() {
-        tryThisThing({ yellThisThing: "something!" });
-        change();
-    }
+  function event() {
+    tryThisThing({ yellThisThing: "something!" });
+    change();
+  }
 
-    function tryThisThing(input: Test) {
-        console.log(input.yellThisThing.toUpperCase());
-    }
+  function tryThisThing(input: Test) {
+    console.log(input.yellThisThing.toUpperCase());
+  }
 
-    let inputBinding: HTMLInputElement;
+  let inputBinding: HTMLInputElement;
 
-    function change() {
-        inputBinding.value = "testing" + Math.round(Math.random() * 100);
-    }
+  function change() {
+    inputBinding.value = "testing" + Math.round(Math.random() * 100);
+  }
 </script>
 
-<style lang="scss">
-    @use "sourcemapImport.scss";
-    @use "sass:color";
-    .sourcemap {
-        $primary-color: #6b717f;
-        color: $primary-color;
-        border: 1px solid color.scale($primary-color, $lightness: 20%);
-    }
-</style>
-
 <button on:click={event} class="sourcemap">Yell "something"</button>
 <input type="text" name="testingInput" bind:this={inputBinding} />
+
+<img src="foo.jpg" />
+
+<style lang="scss">
+  @use "sourcemapImport.scss";
+  @use "sass:color";
+  .sourcemap {
+    $primary-color: #6b717f;
+    color: $primary-color;
+    border: 1px solid color.scale($primary-color, $lightness: 20%);
+  }
+</style>

test/fixtures/preprocessing-sourcemaps/svelte.config.js

@@ -0,0 +1,6 @@
+const { typescript } = require("svelte-preprocess-esbuild");
+const { sass } = require("svelte-preprocess-sass");
+
+module.exports = {
+    preprocess: [{ style: sass() }, typescript()],
+};

test/sourcemapsTest.mjs

@@ -16,8 +16,12 @@ test("Preprocessor Sourcemap test", async () => {
         sourcemap: "external",
         outdir: "out",
         plugins: [sveltePlugin({ preprocess: [{ style: sass() }, typescript()] })],
+        logLevel: "silent",
     });
 
+    assert.equal(result.warnings.length, 1, "Should one warning"); // because of warnings tests
+    assert.equal(result.errors.length, 0, "Should not have errors");
+
     assert.equal(result.outputFiles.length, 4);
 
     for (let out of result.outputFiles) {