micromark / micromark-extension-mdxjs-esm

micromark extension to support MDX JS import/exports
https://unifiedjs.com
MIT License
14 stars 0 forks source link

Is it possible to supplement this error with the actual line from the source code? #2

Closed revelt closed 2 years ago

revelt commented 2 years ago

Initial checklist

Problem

Remix.js uses esbuild to process MDX-based routes using a plugin which uses this package.

Imagine a scenario: a user messes up an export in MDX, for example, omits the "const":

---
title: Test
---
export WillFail = () => (
  <div>won't work</div>
)

The syntax.js will report acorn threw the error and it will report the correct locations, however, all "users" up the chain will not properly tackle the error, and as a result, only the first argument from throw new VFileMessage(... will "bubble up":

Screenshot 2022-06-08 at 21 03 58

No mention of the file, or anything, just a cryptic message.

Solution

I wonder, would it be possible to supplement the syntax.js line 171 ("Could not parse import/exports with acorn") to actually include the whole string value of the line that "result.error.loc.line" references in the second argument passed to VFileMessage:

https://github.com/micromark/micromark-extension-mdxjs-esm/blob/main/dev/lib/syntax.js#L171

to make it like this:

error: Could not parse import/exports with acorn: SyntaxError: Unexpected token -
export WillFail = () => (
^^^^^^^^^^^^^^^^^^^^^^^^^

What if we added a line break and printed out the culprit line?

Alternatives

Well, theoretically, all consumers like Remix.js could beef up their error logs, wrap more try-catch and so on, — but practically, that's unlikely, given the advanced structure of the whole ecosystem of MDX-related plugins.

wooorm commented 2 years ago

Hey!

No mention of the file, or anything, just a cryptic message.

That’s esbuild.

What if we added a line break and printed out the culprit line?

That would be duplicating the work that other projects are supposed to do. I understand you want it, but other projects already deal with it. Them not dealing with it is the problem here.


I don’t think this relates to this project, but to esbuild itself or to the MDX esbuild loader: https://github.com/mdx-js/mdx/blob/63fd208d53f13e5b2451c9f9bc1b7fd136d22c71/packages/esbuild/lib/index.js#L159-L192

As far as I’m aware, that code should do exactly what you want. Perhaps you can debug locally why that isn’t working for you, or make a small reproduction?

revelt commented 2 years ago

Thank you for blazing-quick response.

I found my bug in MDX within 5 minutes, so no worries my side. I'm more concerned about the users in general, how to improve.

Titus, feel free to close this ticket if it's not the right place.

wooorm commented 2 years ago

I share your concern, so I’d appreciate it if you could look into esbuild and @mdx-js/esbuild. Closing as this belongs somewhere else!