feat: expose error length and raw error message on ParseError #3820

mangerlahn · 2023-05-15T12:52:10Z

Add length and rawError as public parameters of ParseError to allow for richer error UIs in editors. This information was already there, but only encoded into the errors message. This change is purely additional, the existing parameters have not changed.

What is the previous behavior before this PR?
ParseError consists of a position (integer) and message (string) that describe the error. Inside the message string, the position, length (through underline) and actual cause are encoded.

What is the new behavior after this PR?
The existing behavior stays the same, ParseError gains two new properties, length (integer) and rawError (string). length together with the existing position describes the range affected by the error. rawError exposes the underlying error message, without the formatting that is added within ParseError.

Reason
We currently implement formula editing and rendering in our app and found this information useful yet missing from the current implementation.

This is what we are currently doing. Showing the raw error below the code while underlining the affected text.

Testing
Since this only exposes existing values, the behavior has not changed. I am happy to update the tests in error-specs.js to both check for position and length of the error. However, I did not do that yet because both values are indirectly tested through the message string (rendering the underline).

edemaine

Sorry for the delay in reviewing this. This looks great, and useful, and the changes are minimal.

Please tweak the comment placement (perhaps on their own line) to satisfy the line length lint.

Regarding testing, please add one or two tests that check that these properties are properly set on the returned error (perhaps where position is already tested, if any). I don't think we need to check the properties for every erroring test, just in a couple of examples so that we ensure that these properties accidentally don't get set in a future regression.

src/ParseError.js

edemaine · 2023-05-24T17:36:14Z

src/ParseError.js

-        // Error position based on passed-in Token or ParseNode.
+    position: number | void;    // Error position based on passed-in Token or ParseNode.
+    length: number | void;      // Length of affected text based on passed-in Token or ParseNode.
+    rawError: string | void;    // The underlying error message without any context added.


I wonder about this property name. Would rawMessage make more sense, given that the contextualized message becomes message?

mangerlahn · 2023-05-31T08:42:10Z

Thanks for the review! I did not find any tests specific to property access, so I added some to katex-spec.js. Not sure, if that is the correct place though.

edemaine · 2023-06-06T15:54:59Z

src/ParseError.js

@@ -66,6 +71,8 @@ class ParseError {
        self.__proto__ = ParseError.prototype;
        // $FlowFixMe
        self.position = start;
+        self.length = end - start;


It occurs to me that this computes NaN if start and end are undefined. Indeed, Flow caught this as well. Perhaps this line should be wrapped in if (start != null && end != null)?

Apparently we also need // $FlowFixMe because self is typed as Error here. But maybe this could be fixed via type casting? (In TypeScript it'd be as.)

(You can run yarn test to run Lint, Jest, and Flow yourself.)

Ah okay, I always just used yarn test:jest, makes more sense now 😅

I tried to cast this error, but that does not seem to help. I get syntax errors, so I assume that this is not TypeScript?
Sorry, I am super unexperienced with JS.

Using // $FlowFixMe works fine.

Right, this is Flow, not TypeScript. Perhaps you can try a Type Cast Expression (: with parens instead of as)?

Thanks for the hint! However this all does not appear to work. As far as I can see, ParseError and Error are distinct classes, having nothing to do with each other. So if I cast self to ParseError, I just get errors when returning self from the constructor, which expects Error. So it's just the other way around. I followed the StackOverflow link at the top of the class declaration. The top answer provides an alternative available since ES6 by just extending Error. This would allow to remove some of the hackery around ParseError.

However, lint is unhappy with this, because extending classes appears to be prohibited in the eslintrc file (ClassDeclaration[superClass]).

If I remove this restriction, the tests appear to run fine on my machine 😅

edemaine · 2023-06-06T15:59:31Z

test/katex-spec.js

+            expect(error.message).toBe("KaTeX parse error: \\verb ended by end of line instead of matching delimiter");
+            expect(error.rawMessage).toBe("\\verb ended by end of line instead of matching delimiter");
+            expect(error.position).toBeUndefined;
+            expect(error.length).toBeUndefined;


I think these need to be toBeUndefined(), which is why the length NaN bug wasn't detected.

mangerlahn · 2023-06-14T14:37:52Z

test/katex-spec.js

+            katex.renderToString("1 + \\fraq{}{}");
+
+            // Render is expected to throw, so this should not be called.
+            fail();


The linter throws an error on the fail command I used in the tests, because this function appears to be defined in a different package called Jasmine. I could add that to the "env" section within the .eslintrc file (appears to work), but I'm not sure if that's desired.

Alternatively I could replace fail with some other failing check like expect(true).toBe(false). What do you prefer?

Either is fine with me! I didn't know about fail so it was neat to learn. But expect... might be more clear to those who also don't know fail. 🙂

mangerlahn · 2023-06-22T07:30:11Z

I just updated the PR, maybe it's better to see some code rather than discussing this in the comments. ParseError now extends Error, not sure if that's desired. I isolated this change in its own commit for now.

codecov · 2023-06-22T23:31:08Z

Codecov Report

Merging #3820 (7426bba) into main (5dd9bc4) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7426bba differs from pull request most recent head c302c9f. Consider uploading reports for the commit c302c9f to get more accurate results

@@           Coverage Diff           @@
##             main    #3820   +/-   ##
=======================================
  Coverage   92.98%   92.98%           
=======================================
  Files          91       91           
  Lines        6770     6770           
  Branches     1574     1575    +1     
=======================================
  Hits         6295     6295           
  Misses        437      437           
  Partials       38       38

Impacted Files	Coverage Δ
src/ParseError.js	`100.00% <100.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dd9bc4...c302c9f. Read the comment docs.

edemaine · 2023-06-22T23:41:53Z

I just updated the PR, maybe it's better to see some code rather than discussing this in the comments. ParseError now extends Error, not sure if that's desired. I isolated this change in its own commit for now.

This code is so much cleaner... However, I read https://stackoverflow.com/q/1382107 a bunch (linked from the source here and from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error) and it looks like class ParseError extends Error cannot be reasonably transpiled for IE 11. Currently, IE 11 is listed as an officially supported platform of KaTeX. That said, there's some good reason to remove that support, mentioned in #2384, and given that IE 11 has been end-of-life for over a year (though you can still download it). Thoughts from anyone on this? I do like how much cleaner this code is, but the code was also OK as it was before...

edemaine · 2023-06-22T23:49:00Z

By the way, I took a crack at making the old code compatible with Flow. Here's the best I came up with, which uses "only" two $FlowFixMes.

class ParseError {
    name: "ParseError";
    ...
    constructor(
        message: string,               // The error message
        token?: ?Token | AnyParseNode, // An object providing position information
    ): ParseError {
        ...
        // $FlowFixMe
        const self: ParseError = new Error(error);
        self.name = "ParseError";
        // $FlowFixMe
        self.__proto__ = ParseError.prototype;
        self.position = start;
        if (start != null && end != null) {
            self.length = end - start;
        }
        self.rawMessage = message;
        return self;
    }
}

I'm still not sure which version is best though, as described in my previous message.

mangerlahn · 2023-06-23T08:36:57Z

Either is fine by me (again having little experience with web dev). If IE11 support is blocking the first solution, I don't see a reason to not use your second version. Could still be changed when that support is removed, right?

edemaine · 2023-06-23T11:31:43Z

Yes. Perhaps we could merge the IE 11 compatible version now, and make separate cleanup PR for dropping IE 11?

Add length and rawError as public parameters of ParseError to allow for richer error UIs in editors. This information was already there, but only encoded into the errors message. This change is purely additional, the existing parameters have not changed.

mangerlahn · 2023-06-23T12:22:43Z

Thanks for your patience! 😃

edemaine · 2023-06-23T12:22:51Z

Sorry, one more thing: can you update against the current main branch? Merge is fine; we squash PRs. (You don't seem to have given me permission to do so.)

## [0.16.8](v0.16.7...v0.16.8) (2023-06-24) ### Features * expose error length and raw error message on ParseError ([#3820](#3820)) ([710774a](710774a))

KaTeX-bot · 2023-06-24T05:30:22Z

🎉 This PR is included in version 0.16.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

edemaine requested changes May 24, 2023

View reviewed changes

mangerlahn force-pushed the feature/error-information branch from 0468d1a to 9d32f00 Compare May 31, 2023 08:37

edemaine requested changes Jun 6, 2023

View reviewed changes

mangerlahn commented Jun 14, 2023

View reviewed changes

mangerlahn force-pushed the feature/error-information branch from 9d32f00 to 7426bba Compare June 22, 2023 07:22

mangerlahn force-pushed the feature/error-information branch from 7426bba to 14fef5b Compare June 23, 2023 12:00

edemaine approved these changes Jun 23, 2023

View reviewed changes

edemaine enabled auto-merge (squash) June 23, 2023 12:21

edemaine disabled auto-merge June 23, 2023 12:21

Merge branch 'main' into feature/error-information

c302c9f

edemaine enabled auto-merge (squash) June 24, 2023 05:26

edemaine merged commit 710774a into KaTeX:main Jun 24, 2023
7 checks passed

KaTeX-bot added a commit that referenced this pull request Jun 24, 2023

chore(release): 0.16.8 [ci skip]

ad03d1e

## [0.16.8](v0.16.7...v0.16.8) (2023-06-24) ### Features * expose error length and raw error message on ParseError ([#3820](#3820)) ([710774a](710774a))

KaTeX-bot added the released label Jun 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose error length and raw error message on ParseError #3820

feat: expose error length and raw error message on ParseError #3820

mangerlahn commented May 15, 2023

edemaine left a comment

edemaine May 24, 2023

mangerlahn commented May 31, 2023

edemaine Jun 6, 2023

mangerlahn Jun 14, 2023

mangerlahn Jun 14, 2023

edemaine Jun 14, 2023

mangerlahn Jun 15, 2023

edemaine Jun 6, 2023

mangerlahn Jun 14, 2023 •

edited

edemaine Jun 14, 2023

mangerlahn commented Jun 22, 2023

codecov bot commented Jun 22, 2023 •

edited

edemaine commented Jun 22, 2023

edemaine commented Jun 22, 2023

mangerlahn commented Jun 23, 2023

edemaine commented Jun 23, 2023

mangerlahn commented Jun 23, 2023

edemaine commented Jun 23, 2023

KaTeX-bot commented Jun 24, 2023

feat: expose error length and raw error message on ParseError #3820

feat: expose error length and raw error message on ParseError #3820

Conversation

mangerlahn commented May 15, 2023

edemaine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangerlahn commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangerlahn Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangerlahn commented Jun 22, 2023

codecov bot commented Jun 22, 2023 • edited

Codecov Report

edemaine commented Jun 22, 2023

edemaine commented Jun 22, 2023

mangerlahn commented Jun 23, 2023

edemaine commented Jun 23, 2023

mangerlahn commented Jun 23, 2023

edemaine commented Jun 23, 2023

KaTeX-bot commented Jun 24, 2023

mangerlahn Jun 14, 2023 •

edited

codecov bot commented Jun 22, 2023 •

edited