Improve elm compiler error output

[ChristophP]

↪️ Pull Request

Fixes #7957

This PR adds changes to the transformer-elm that makes compiler output look more like they used to in Parcel v1.
The way it is currently is hard to read in many terminals with a dark background.

💻 Examples

This implements to following change, when any compilation error happens for Elm code. The output is more readable because it is logged as in the message of the diagnostic instead of the stack part.

Before:

After:

🚨 Test instructions

This PR also adds an entry in the packages/examples for folder elm for testing. It was useful for me and I thought that was the way to go from reading the Contributing.md file. If that example is not useful to anyone but me I can remove it from this PR if anyone lets me know.

Testable this way:

• cd into packages/examples/elm
• run yarn install
• delete random lines from src/Main.elm until the error output appears in the console.
• run yarn demo

✔️ Bonus

While this change improves the error output in the terminal, it kind of messes up the one in the browser.

With this change:

Before this change (correctly formatted but hard to read if background is dark also a gray tone):

This is due to the browser display not respecting white space for formatting.

Would it be possible to add white-space: pre; to the .error-message class? It seems that whitespace is preserved when logging things as stack (but hard to read on dark background) but not for the message part of a throwable diagnostic. I think it would make sense to add this because otherwise you get things that look differently in the terminal and in the browser.

[mischnic]

Ideally we could extract the file/line/column/message parts and then create a diagnostic codeframe, but that's probably not exposed separately in the error object.

[ChristophP]

@mischnic Thanks for checking.

Short answer

The elm compiler error does provide file/line/column info but the problem is that the message contains code/hint parts with no way to extract them in a way that makes sense. Generally the error structure of Elm does not map very well to the Parcel diagnostic structure, that's why I only tried to improve the way it is displayed as a message in the diagnostic.

Would leaving it this way be ok or is there another way to make it work? Any input is appreciated.

Long answer

I attempted to get it to work, but as far as I'm concerned I couldn't get converted in a meaningful way.

Here's an example for two errors:

Here's the JSON version for this message. As you can see the `message` is simply an array of pieces with info what the **hint** or the **code** is. Click here ...

The interesting part is at $.errors[].problems[].message.

{
  "type": "compile-errors",
  "errors": [
    {
      "path": "/Users/d441365/projects/test/src/Main.elm",
      "name": "Main",
      "problems": [
        {
          "title": "MISSING PATTERNS",
          "region": {
            "start": {
              "line": 26,
              "column": 5
            },
            "end": {
              "line": 31,
              "column": 48
            }
          },
          "message": [
            "This `case` does not have branches for all possibilities:\n\n26|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "    case msg of\n27|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "        Increment ->\n28|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "            { model | count = model.count + 1 }\n29|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "\n30|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "        Decrement ->\n31|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "            { model | count = model.count - 1 }\n\nMissing possibilities include:\n\n    ",
            {
              "bold": false,
              "underline": false,
              "color": "yellow",
              "string": "Peter"
            },
            "\n\nI would have to crash if I saw one of those. Add branches for them!\n\n",
            {
              "bold": false,
              "underline": true,
              "color": null,
              "string": "Hint"
            },
            ": If you want to write the code for each branch later, use `Debug.todo` as a\nplaceholder. Read <https://elm-lang.org/0.19.1/missing-patterns> for more\nguidance on this workflow."
          ]
        },
        {
          "title": "MISSING PATTERNS",
          "region": {
            "start": {
              "line": 34,
              "column": 5
            },
            "end": {
              "line": 36,
              "column": 29
            }
          },
          "message": [
            "This `case` does not have branches for all possibilities:\n\n34|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "    case msg of\n35|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "        Increment -> \"Assi\"\n36|",
            {
              "bold": false,
              "underline": false,
              "color": "RED",
              "string": ">"
            },
            "        Decrement -> \"Peter\"\n\nMissing possibilities include:\n\n    ",
            {
              "bold": false,
              "underline": false,
              "color": "yellow",
              "string": "Peter"
            },
            "\n\nI would have to crash if I saw one of those. Add branches for them!\n\n",
            {
              "bold": false,
              "underline": true,
              "color": null,
              "string": "Hint"
            },
            ": If you want to write the code for each branch later, use `Debug.todo` as a\nplaceholder. Read <https://elm-lang.org/0.19.1/missing-patterns> for more\nguidance on this workflow."
          ]
        }
      ]
    }
  ]
}

As you can see the message is basically a sentence that uses code snippets sometimes in the middle of a sentence.
I guess unlike other tools, the coupling between message, code snippet and hint in Elm is much higher.

On a high level the parcel diagnostic contains

• 1 message
• multiple code frames (which can contain 0..* code highlights which can each have a message)
• 1 hint

The Elm error output contains (per problem)

• 1 code frame
• 1 message, which already contains the code snippet in a formatted way
• no structurally separate hint, the hint is part of the message and there can be multiple hints in some cases.

Since there is no structural information to separate the hint and the code frame from the message, and the fact that the message already contains both in a meaningful way, makes it hard to map to parcel's model, so I'm suggesting to leave it at that.

For reference:

This merged PR also decided to leave the message as a string after stuggling with mapping it to a diagnostic.

I looked into using codeframes to get syntax highlighting. Running the elm compiler with --report=json gives usable diagnostic location information, but the error message is still quite verbose, including code snippets (again) and hints. It just didn't look right, so I've left it at printing the stack trace.

[mischnic]

Yeah, putting this into the message instead of the stack is definitely an improvement over the current situation.

Please remove the example and create a test case similar to one of these (so also create a folder next to integration/elm with invalid elm that fails to compile)

parcel/packages/core/integration-tests/test/elm.js

Line 12 in fe58766

it('should produce a basic Elm bundle', async function () {

Then you can assert the diagnostic like this

parcel/packages/core/integration-tests/test/html.js

Line 2751 in fdeb508

it('should throw error with empty string reference to other resource', async function () {

---

That leading space removal comes from this wrapWithIndent call:

parcel/packages/reporters/cli/src/CLIReporter.js

Line 184 in fe58766

writeOut(wrapWithIndent(message), isError);

This could be fixed by changing it to this to only trim trailing spaces

function wrapWithIndent(string, indent = 0, initialIndent = indent) {
  let width = getTerminalWidth().columns;
  return indentString(
    wrapAnsi(string.trimEnd(), width - indent, {trim: false}),
    indent,
    initialIndent,
  );
}

---

Would it be possible to add white-space: pre; to the .error-message class?

Yes, let's do that.

[ChristophP]

@mischnic great, I will provide the changes you asked for.

[ChristophP]

@mischnic Made all the changes

• removed the Elm example
• added a test case
• changed the indentation function as suggested
• added white-space: pre to the CSS class for the error message in the page that gets displayed on build fail

Now the output looks nice.

Terminal:

Browser:

[devongovett]

Seems ok to me. Might be nice to try to map these to Parcel diagnostics if we can figure out how at some point, but this is an improvement.

[ChristophP]

Nice, thanks for reviewing.
I see some failing checks. Is there anything I still need to fix or have these been failing for different reasons?

[mischnic]

Mostly that's on us because Some tests are flakey (and we'll take care of that).
However, there is one thing you need to change: this test is failing on Windows

  1) elm
       should produce correct formatting and indentation when compilation fails:
     AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  Comparison {
    diagnostics: [
      {
        message: '\n' +
+         '-- TYPE MISMATCH --------------- test\\integration\\elm-compile-error\\src\\Main.elm\n' +
-         '-- TYPE MISMATCH --------------- test/integration/elm-compile-error/src/Main.elm\n' +
          '\n' +
          'The 1st argument to `text` is not what I expect:\n' +
...
      {
        message: '\n' +
+         '-- TOO MANY ARGS --------------- test\\integration\\elm-compile-error\\src\\Main.elm\n' +
-         '-- TOO MANY ARGS --------------- test/integration/elm-compile-error/src/Main.elm\n' +
          '\n' +
...
    ],
    name: 'BuildError'
  }
      at Context.<anonymous> (test\/elm.js:86:5)

You'll have to do wrap the test/integration/elm-compile-error/src/Main.elm part in packages/core/integration-tests/test/elm.js with normalizePath, for example:

parcel/packages/core/integration-tests/test/scope-hoisting.js

Lines 559 to 562 in 4e40429

	let message = md`${normalizePath(
	'integration/scope-hoisting/es6/re-export-missing/c.js',
	false,
	)} does not export 'foo'`;

[ChristophP]

🙌