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.

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

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

Closed
wants to merge 15 commits into from

Conversation

kdy1
Copy link
Contributor

@kdy1 kdy1 commented Mar 25, 2024

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
Copy link
Owner

wooorm commented Mar 25, 2024

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., #42 (comment)

@kdy1
Copy link
Contributor Author

kdy1 commented Mar 25, 2024

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
Copy link
Owner

wooorm commented Mar 25, 2024

user wants the kind of error

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

@kdy1
Copy link
Contributor Author

kdy1 commented Mar 25, 2024

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

@wooorm
Copy link
Owner

wooorm commented Mar 26, 2024

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
Copy link
Contributor Author

kdy1 commented Mar 27, 2024

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.

#42 (comment)

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
Copy link
Contributor Author

kdy1 commented Mar 27, 2024

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
Copy link
Contributor Author

kdy1 commented Mar 27, 2024

@wooorm
Copy link
Owner

wooorm commented Mar 27, 2024

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

Where are you parsing errors?

@kdy1
Copy link
Contributor Author

kdy1 commented Mar 27, 2024

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

@kdy1
Copy link
Contributor Author

kdy1 commented Apr 1, 2024

@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 kdy1 changed the title [WIP]: Make errors typed feat: Use a typed error to allow getting location without parsing Apr 2, 2024
@kdy1 kdy1 marked this pull request as ready for review April 2, 2024 07:33
@kdy1
Copy link
Contributor Author

kdy1 commented Apr 2, 2024

I updated the PR description

wooorm added a commit to wooorm/markdown-rs that referenced this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants