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

Flip order of error message for wrong name #6336

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

zth
Copy link
Collaborator

@zth zth commented Aug 4, 2023

This flips the order of the error message parts for the "wrong name" error message. This is to make things clearer. Instead of starting with the definition of the thing we're looking for, we start with what's wrong, and then print the definition.

This makes things clearer in general, but it especially improves the situation for React components with a large amount of props, or with props with types that are verbose.

React component before:

This record expression is expected to have type
    Component.props<
  string,
  string,
  string,
  string,
  string,
  string,
  string,
  string,
  string,
>
  The field nonExistant does not belong to type Component.props

After:

The field nonExistant does not belong to type Component.props
  
  This record expression is expected to have type
    Component.props<
  string,
  string,
  string,
  string,
  string,
  string,
  string,
  string,
  string,
>

The biggest improvement is seen in the editor, where the relevant information is now visible without needing to scroll in the hover or problems tab:

image

@zth zth requested review from cristianoc and mununki August 4, 2023 07:53
@DZakh
Copy link
Contributor

DZakh commented Aug 4, 2023

How about wrapping the field name with quotes? I think it'll make the error message more readable. Now the field name blends in with the rest of the text. Here's the example from TS:
image

@tsnobip
Copy link
Contributor

tsnobip commented Aug 4, 2023

I'm a bit more doubtful about this one, the order is indeed more logical this way, but having the important piece of information last in the terminal is also nice.

@zth
Copy link
Collaborator Author

zth commented Aug 4, 2023

I'm a bit more doubtful about this one, the order is indeed more logical this way, but having the important piece of information last in the terminal is also nice.

Yeah, it's a good point, and a trade off. I've optimized for the editor experience here, where what's on top matters:

image

@zth
Copy link
Collaborator Author

zth commented Aug 4, 2023

How about wrapping the field name with quotes? I think it'll make the error message more readable. Now the field name blends in with the rest of the text. Here's the example from TS: image

Definitively possible to do. Although they are colored in the terminal, but I don't believe they're colored in the editor.

@tsnobip
Copy link
Contributor

tsnobip commented Aug 4, 2023

Yeah, it's a good point, and a trade off. I've optimized for the editor experience here, where what's on top matters:

image

It indeeds looks much better in the IDE

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

If there's enough consensus, go for it.

@JasoonS
Copy link

JasoonS commented Aug 5, 2023

I typically read the error block from top to bottom in my terminal, so I approve of this change 👍

@zth
Copy link
Collaborator Author

zth commented Aug 5, 2023

Waiting for more people to weigh in on what they think about this before deciding whether to go with it.

@DZakh
Copy link
Contributor

DZakh commented Aug 5, 2023

I like that The field nonExistant does not belong to type Component.props and This record expression is expected to have type became close to each other.

@zth zth force-pushed the error-message-flip-wrong-name-message-order branch from 15c1ffb to 6600cbd Compare August 7, 2023 17:42
@zth zth merged commit e3bc40d into master Aug 7, 2023
2 checks passed
@zth zth deleted the error-message-flip-wrong-name-message-order branch August 7, 2023 17:42
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.

None yet

5 participants