Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling improvements to esbuild plugin #2595

Merged
merged 11 commits into from
Mar 7, 2025
Merged

Error handling improvements to esbuild plugin #2595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3d2e68e
improve error reporting from esbuild plugin (WIP)
egnor Feb 27, 2025
d3d13e1
fix coverage
egnor Mar 3, 2025
199439e
fix tsc errors
egnor Mar 3, 2025
ed891fc
Update packages/esbuild/lib/index.js
egnor Mar 4, 2025
377c6e4
Update packages/esbuild/test/index.js
egnor Mar 4, 2025
0b85289
Update packages/esbuild/test/index.js
egnor Mar 4, 2025
bbe4f87
merge cause into message when converting to esbuild message, not when…
egnor Mar 4, 2025
2b32e26
avoid instanceof for imported types
egnor Mar 4, 2025
3dffdcf
fix tsc
egnor Mar 4, 2025
a89028b
ran npm test
egnor Mar 5, 2025
d3c901f
simplify cause reporting
egnor Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 51 additions & 36 deletions packages/esbuild/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* @import {CompileOptions} from '@mdx-js/mdx'
* @import {
Location,
Message,
OnLoadArgs,
OnLoadResult,
Expand Down Expand Up @@ -117,14 +118,15 @@ export function esbuild(options) {
messages = file.messages
} catch (error_) {
const cause = /** @type {VFileMessage | Error} */ (error_)
const message =
'reason' in cause
? cause
: new VFileMessage('Cannot process MDX file with esbuild', {
cause,
ruleId: 'process-error',
source: '@mdx-js/esbuild'
})
const message = new VFileMessage(
'Cannot process MDX file with esbuild',
{
cause,
place: 'reason' in cause ? cause.place : undefined,
ruleId: 'process-error',
source: '@mdx-js/esbuild'
}
)
message.fatal = true
messages.push(message)
}
Expand Down Expand Up @@ -157,44 +159,57 @@ export function esbuild(options) {
* ESBuild message.
*/
function vfileMessageToEsbuild(state, message) {
/** @type {Location} */
const location = {
column: 0,
file: state.path,
length: 0,
line: 0,
lineText: '',
namespace: 'file',
suggestion: ''
}

const place = message.place
const start = place ? ('start' in place ? place.start : place) : undefined
const end = place && 'end' in place ? place.end : undefined
let length = 0
let lineStart = 0
let line = 0
let column = 0

if (start && start.offset !== undefined) {
line = start.line
column = start.column - 1
lineStart = start.offset - column
length = 1

if (end && end.offset !== undefined) {
length = end.offset - start.offset
if (start) {
location.column = start.column - 1
location.line = start.line
location.length = 1

const end = place && 'end' in place ? place.end : undefined
if (end) {
if (start.offset !== undefined && end.offset !== undefined) {
location.length = end.offset - start.offset
} else if (end.line === start.line) {
location.length = end.column - start.column
}
}
}

eol.lastIndex = lineStart
if (start.offset !== undefined) {
eol.lastIndex = start.offset
const match = eol.exec(state.doc)
const lineStart = start.offset - (start.column - 1)
const lineEnd = match ? match.index : state.doc.length
location.lineText = state.doc.slice(lineStart, lineEnd)
location.length = Math.min(location.length, lineEnd - (start.offset || 0))
}

const maxLength = state.doc.length - (start.offset || 0)
location.length = Math.min(location.length, maxLength)
}

const match = eol.exec(state.doc)
const lineEnd = match ? match.index : state.doc.length
let text = message.reason
if (message.cause) {
text = `${text}:\n ${message.cause}`
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these lines because otherwise the “cause” is lost? That ESBuild doesn’t print them?

Is that something that ESbuild should fix? Upstream?

Or, if indeed needed here, perhaps you could use vfile-reporter I linked above, or, perhaps you could use inspect from util: https://nodejs.org/api/util.html#utilinspectobject-options. I believe that it serializes recursive causes

Copy link
Contributor Author

@egnor egnor Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yes this is the tricky bit.

In esbuild you can report errors by just throwing an exception, or by adding to a list of messages using their own Message class (which is what we're doing). That class includes a detail field which can be an exception but could also be anything else, and in any case doesn't get printed in esbuild's output, it seems to be for custom API use; it's not really comparable to cause.

Alternatives

  • use vfile-reporter -- this does a fine job and does chase cause chains, unfortunately this wants an entire VFile, I don't see a way to format a single VFileMessage (though you could imagine exposing that)?
  • use util.inspect -- this has a lovely formatter for Error objects, but even for subclasses like VFileMessage reverts to its default printer which just dumps the fields and does not print error messages or chase cause chains?
  • just let the error get thrown rather than catching it and adding it to messages -- this does get handled by esbuild gracefully (including plugin attribution), but the extra location information in VFileMessage is lost
  • add VFileMessage-shaped exceptions directly to messages (no wrapping), re-throw everything else, remove all cause-chain-processing -- this could work??

For now I simplified the cause processing to not try to walk the whole cause chain but just include the string representation of the cause if one exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, I appreciate that you came up with alternatives & discussed trade offs.

I could see an reportError in vfile-reporter, which can then be used here.

The no-wrapping idea could also perhaps work.

But this is probably fine to start with!


return {
detail: message,
id: '',
location: {
column,
file: state.path,
length: Math.min(length, lineEnd),
line,
lineText: state.doc.slice(lineStart, lineEnd),
namespace: 'file',
suggestion: ''
},
location,
notes: [],
pluginName: state.name,
text: message.reason
text
}
}
23 changes: 11 additions & 12 deletions packages/esbuild/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ test('@mdx-js/esbuild', async function (t) {
},
notes: [],
pluginName: '@mdx-js/esbuild',
text: 'Unexpected character `/` (U+002F) before local name, expected a character that can start a name, such as a letter, `$`, or `_` (note: to create a link in MDX, use `[text](url)`)'
text: 'Cannot process MDX file with esbuild:\n 1:12: Unexpected character `/` (U+002F) before local name, expected a character that can start a name, such as a letter, `$`, or `_` (note: to create a link in MDX, use `[text](url)`)'
})
}

Expand Down Expand Up @@ -271,7 +271,7 @@ test('@mdx-js/esbuild', async function (t) {
location: {
column: 20,
file: 'test/esbuild.mdx',
length: 1,
length: 0,
line: 3,
lineText: '# Hello, <Message />',
namespace: 'file',
Expand All @@ -290,7 +290,7 @@ test('@mdx-js/esbuild', async function (t) {
file: 'test/esbuild.mdx',
length: 0,
line: 0,
lineText: 'export function Message() { return <>World!</> }',
lineText: '',
namespace: 'file',
suggestion: ''
},
Expand All @@ -305,7 +305,7 @@ test('@mdx-js/esbuild', async function (t) {
file: 'test/esbuild.mdx',
length: 0,
line: 0,
lineText: 'export function Message() { return <>World!</> }',
lineText: '',
namespace: 'file',
suggestion: ''
},
Expand Down Expand Up @@ -365,7 +365,7 @@ test('@mdx-js/esbuild', async function (t) {
file: 'test/esbuild.mdx',
length: 11,
line: 3,
lineText: '# Hello, <Message />',
lineText: '',
namespace: 'file',
suggestion: ''
},
Expand Down Expand Up @@ -414,7 +414,10 @@ test('@mdx-js/esbuild', async function (t) {
file.message('3', tree)
file.message('4', esm)
file.message('5', text)
file.message('6', jsx)
const m6 = file.message('6', jsx)
assert(m6.place)
assert('start' in m6.place)
delete m6.place.start.offset
file.message('7', head.position.end).fatal = true // End of heading
}
}
Expand All @@ -439,10 +442,6 @@ test('@mdx-js/esbuild', async function (t) {
/** @type {BuildFailure} */
const result = JSON.parse(JSON.stringify(error))

for (const message of [...result.errors, ...result.warnings]) {
message.text = message.text.split('\n')[0]
}

assert.deepEqual(result, {
errors: [
{
Expand All @@ -461,13 +460,13 @@ test('@mdx-js/esbuild', async function (t) {
file: 'test/esbuild.mdx',
length: 0,
line: 0,
lineText: '# hi',
lineText: '',
namespace: 'file',
suggestion: ''
},
notes: [],
pluginName: '@mdx-js/esbuild',
text: 'Cannot process MDX file with esbuild'
text: 'Cannot process MDX file with esbuild:\n Error: Something went wrong'
}
],
warnings: []
Expand Down