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

Display flow errors like on flow.org #101

Open
nodkz opened this issue Jun 28, 2017 · 0 comments
Open

Display flow errors like on flow.org #101

nodkz opened this issue Jun 28, 2017 · 0 comments

Comments

@nodkz
Copy link

nodkz commented Jun 28, 2017

Let's take a look on the first example from https://flow.org/en/docs/types/maybe/ or any other live example from this site.

screen shot 2017-06-28 at 12 33 21

screen shot 2017-06-28 at 12 33 30

Both places (type definition and arg value) are hightlighet with error. Cause Flow does not know if you make error in type definition or in value declaration. So it's better to highlight errors by all locations which flow provides in message[] (open JSON tab in REPL) for exact error.

Now one IDE for now (flow-ide, nuclide, someone vscode plugin) does not highlight error in all places.

The worst experience what I getting is when I working with React Components covered by flow. If I provide incorrect property to component, error highlighted in Component declaration (in other file). Not in that file in which was written incorrect property. An it is sad.


Brings here previous conversation from #94 (comment)

@nodkz

Can we improve this plugin by displaying errors in several locations (different files)?

I use Flow for React components, and when I miss required property for component (in SwipeMagicRenderer.js)

screen shot 2017-06-21 at 20 10 29

the error displays only in a file SwipeableResizer.js where the component is defined:

screen shot 2017-06-21 at 20 11 01

Flowtype provides the following information for this Error (full json output gist):

{
  message: [
    { loc: { source: '..../SwipeableResizer.js' } },
    { loc: undefined },
    { loc: { source: '..../SwipeMagicRenderer.js' } },
  ]
}

But in function toLinterReference it takes only first file. But for better developer experience will be better to display error in all locations:

function toLinterReference(messages) {
  for (let i = 1, length = messages.length; i < length; i++) {
    const message = messages[i]
    if (message.loc) {
      return {
        file: message.loc.source,
        position: locationToRange(message.loc)[0],
      }
    }
  }
  return null
}

PS. Similar issue for Nuclide I opened 1 year ago, but they did not understand me.

PSS flow-ide plugin is much-much better. And can be better even more. Thanks for such awesome thing!

@steelbrain

I had implemented what you're saying in my Atom Hack package, which's linting part was seperated and is now the linter package, I dropped it for plenty of reasons.

The first and most important one is how do you distinguish between a normal message and a part of any other message? Keep in mind that blue underlines are already part of Info messages, also what do we show there? All of the message or just the relevant part? If all, how's the user going to know which part is the current one.

The second most important reason is performance, every new added message will be searched for references for not just the current text editor but messages from all text editors and will be added/removed when the text editor is closed and opened up again so this scales pretty horribly.

Also the support for standardized traces was removed in Linter Messages v2 to allow for a markdown string so v2 providers are inherently incapable of such a thing

@lloiser

@nodkz I would agree with you if flow would provide a better error message for each entry in message. Unfortunately flow's errors often only make sense if you read the complete error which is now visible in the error description.
I will merge this now. If you still think your suggestion is valid please open another issue and/or a pull request.

Brain, Lukas I mean that whole error (combined from all message entries like it already did in #94) will be displayed by all locations (unique locations gathered from all entries). Like it flowtype.org does (see first two screenshoots). And yep, I'm ready to Atom performance degradation, in favor forgetting calling flow via terminal and exploring whole error and all locations there. BTW this feature (highlight error in all places) can be hidden by option for this atom plugin. In all my apps and OSS packages I have 0 flowtype errors, so with this feature I will not have perf degradation, but obtain better dev exp.

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

No branches or pull requests

1 participant