wooorm / mdxjs-rs

Compile MDX to JavaScript in Rust
https://docs.rs/mdxjs/
MIT License
429 stars 14 forks source link

feat: Use a typed error to allow getting location without parsing #43

Closed kdy1 closed 6 months ago

kdy1 commented 7 months ago

I added an error type, which consists of the error message and optional source location.

This would be enough for most users, including usage in next.js/turbopack - providing good error message with some underline to the source code.

wooorm commented 7 months ago

hacky PR

Maybe! What is your goal? What data do you want to access on errors? It’s probably 50/50 whether errors are from the markdown/mdx language, and which are from SWC. Why not an error struct? See the other comments on #42, e.g., https://github.com/wooorm/mdxjs-rs/issues/42#issuecomment-1971795251

kdy1 commented 7 months ago

My goal is generally processing errors using the error handler of the project - the one of swc for next-swx and the own system for turbopack.

I think an Error struct will work, but in general user wants the kind of error. In this case it will be parsing error I think.

wooorm commented 7 months ago

user wants the kind of error

What kind of error? Sorry, I don’t understand.

kdy1 commented 7 months ago

I meant parsing error vs transform error vs etc but maybe those can be merged

wooorm commented 7 months ago

I meant parsing error vs transform error vs etc but maybe those can be merged

I’m not sure what a parsing error vs a transforming error is. Or whether that divide is important. SWC does parsing and gives errors. And markdown-rs does parsing and gives errors. markdown-rs has a hook so that mdxjs-rs can also give parsing errors.

Most errors are parsing errors, 90% vs. 10% probably? Do users care about what is a parse or transform error?


Some more background:

In my comments in the related issue, I proposed matching what we do in JavaScript. The algorithms are very similar. Compare what happens in Rust:

https://github.com/wooorm/markdown-rs/blob/60db8e5896be05d23137a6bdb806e63519171f9e/src/construct/partial_mdx_jsx.rs#L244-L255

With what happens in JavaScript:

https://github.com/micromark/micromark-extension-mdx-jsx/blob/main/dev/lib/factory-tag.js#L181-L188

I think it’s good to keep the code bases similar for maintainers, and also to keep errors similar for users. Like JavaScript, we can also use a struct with different fields.


Q: in your OP you said “To avoid modifying markdown-rs”. Why? Why not create an error struct there, that can also be used here?

kdy1 commented 7 months ago

I think behaving similarly is a good idea.

Q: in your OP you said “To avoid modifying markdown-rs”. Why? Why not create an error struct there, that can also be used here?

It was about using std::error::Error as the error type. I thought you don't want to introduce more std-related features to markdown-rs because of #[no_std] support of it.

https://github.com/wooorm/mdxjs-rs/issues/42#issuecomment-1971795251

markdown-rs is no_std + alloc, it’s intentionally minimal so others can build more on top. That isn’t needed in mdxjs-rs. But, we’d need to find something that works for both.

kdy1 commented 7 months ago

I'm not sure if the user actually cares about error kinds. Maybe it's enough if we don't need to parse the error message. I'll check if there's a major difference between errors

kdy1 commented 7 months ago

How does https://github.com/kdy1/mdxjs-rs/blob/e8c8332db1cc6c7b15b7d4f0025f74e0d7c51ed0/src/error.rs#L16-L27 sound to you?

wooorm commented 7 months ago

Maybe it's enough if we don't need to parse the error message

Where are you parsing errors?

kdy1 commented 7 months ago

I don't (yet), but I want to show good errors to users.

kdy1 commented 7 months ago

@wooorm Is this what you meant by an error struct? This type will be enough for pointing exact cause in the input source, which is the primary goal.

kdy1 commented 7 months ago

I updated the PR description