Skip to content

chore: markdown errors #11294

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

Merged
merged 22 commits into from
Apr 23, 2024
Merged

chore: markdown errors #11294

merged 22 commits into from
Apr 23, 2024

Conversation

Rich-Harris
Copy link
Member

This begins to scratch an itch I've had for a long time. At present, our error and warning handling is a bit ropey — the messages are stored in a messy module, and are generated by passing magic strings around. This PR moves us towards a state I'd consider more ideal:

  • The messages are stored in .md files, and as such are much more readable and easy to edit
  • Longer term, we could take advantage of this to provide much more detail in our errors and warnings. At present we naturally veer towards being stingy in our messages because it's much easier to make everything a one-liner, when really we could be providing rich, detailed messages that explain concepts (think links to documentation and playgrounds, diagrams, ASCII art, you name it) rather than just yelling at users
  • Instead of magic strings, we are calling real functions, with autocomplete and other niceties
  • As a corollary, we can take advantage of things like treeshaking. We can also find unused error messages with things like Knip, etc
  • We can use the same mechanism for runtime errors and warnings, and these can drop the message (in favour of just codes) in production. This is where the treeshaking stuff will become particularly valuable
  • We can generate docs pages from these messages, so that everything is linkable and searchable

Notes:

  • Some of the error codes are honestly a bit weird — there's no clear rationale for when we use invalid vs illegal, and the order in which words go feels a bit arbitrary. We should audit the whole thing, but I didn't want to get into that here, it can be a follow-up
  • Similarly for the way things are categorised — it could use a tidy up
  • The 'Cannot bind to constant' error could use some more detail in cases like let:x or {:then x}, but the best route is to refactor Binding, which is also a bit of a mess (I always get confused about binding.kind vs binding.declaration_kind, and it would be good if there was a binding.constant property)
  • I tweaked a few error messages where it felt appropriate, but I didn't go ham. More to follow
  • error codes are now snake_case rather than kebab-case. Initially this was because it needs to be that way in order to be valid function names, but I actually think it's better, since you can double-click an error code to select the whole thing
  • I plan to do the generated docs stuff in a follow-up
  • Same for compiler warnings, and runtime errors/warnings

This might seem low priority given all the other stuff that needs to happen for Svelte 5, but it's one of those things with compounding dividends, and since it's technically breaking it's good to do it sooner rather than later.

Copy link

changeset-bot bot commented Apr 22, 2024

⚠️ No Changeset found

Latest commit: 7ca500d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dummdidumm
Copy link
Member

Nice! Thoughts:

  • I don't understand the underscore rationale. Where can I more easily click on something? The industry standard is using dashes, it would be nice to keep that
  • it would be great to have the option to have a short version for the compiler error and a long version for the docs. We have that today for accessibility warnings (but manually )
  • the categories feel a bit arbitrary, I agree. Maybe less is better. I don't mind having huge md files

@Rich-Harris
Copy link
Member Author

Where can I more easily click on something?

Say we log this to your terminal:

some_error_message
This is a detailed description of the error

Compare with this:

some-error-message
This is a detailed description of the error

Try double-clicking both.

The industry standard is using dashes

Is there an industry standard for this? Can you give examples?

it would be great to have the option to have a short version for the compiler error and a long version for the docs

Yeah, we could do that with blockquote syntax or something. I didn't want to overcomplicate this PR with anything like that though

@dummdidumm
Copy link
Member

Huh, could've sworn I saw many using dash case but looking up others it's a mix of numbers and camelCase, so nevermind. I still don't fully understand the terminal argument though, what advantage do I have from clicking it and getting the full selection?

@PuruVJ
Copy link
Collaborator

PuruVJ commented Apr 23, 2024

Opinion as a bystander: I prefer _ for the same reason Rich suggested, easy to copy with double click and paste in google to figure out the error. And its also more readable IMO

@dummdidumm
Copy link
Member

Shouldn't we instead have a link to the docs (where we can also get into more detail) as part of the error message? That would make that argument irrelevant.

@Rich-Harris
Copy link
Member Author

We could, though in the cases where there isn't more detail that would be a waste of people's time. And it still wouldn't help in cases where you e.g. needed to squelch a warning by copy-pasting the code into a <!-- svelte-ignore --> comment.

For me the question is less 'why use snake_case?' but 'why use kebab-case?' - it means having a conversion step that ultimately makes it marginally harder to find the markdown source of a particular error, and I'm just not sure I'm seeing any convincing reasons to inflict that on ourselves.

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.

3 participants