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

Red squiggly lines not shown for missing prop in JSX #39

Closed
sgronblo opened this issue Sep 28, 2016 · 12 comments
Closed

Red squiggly lines not shown for missing prop in JSX #39

sgronblo opened this issue Sep 28, 2016 · 12 comments

Comments

@sgronblo
Copy link

No error is shown inside of JSX when trying to render a component without one of its required properties. If I run Flow from the command line I get the following:

src/containers/App/App.js:83
 83:           <Nav
               ^ React element `Nav`
 28:   props: Props
              ^^^^^ property `onSelect`. Property not found in. See: src/components/Nav/Nav.js:28
 83:           <Nav
               ^ props of React element `Nav`

The Nav.js:28 line has a red squiggly, which is good. But it would be even more useful if App.js:83 also had a red squiggly line for <Nav in the JSX.

@utkuturunc
Copy link

I am having this issue.

Instead of showing line in App.js, mine shows error in Nav.js in props: Props line (i changed the names of components to app and nav)

@ghost
Copy link

ghost commented Dec 12, 2016

I partially solved this type of error diagnostics in other vscode extension - see this commit
https://github.com/SaboteurSpk/vscode-flow-ide/commit/2fc4355946aee748015fbe157d8e25298db01da4

You can test in diagnostics result if error has operation with type "Blame"
f(err.operation && err.operation.type === "Blame")

I would like to try to contribute to this repository because the above mentioned extension doesn't work well with other things like find all references, navigate to type definition etc. , but I'm not able to debug this extension - will fill a request issue

@gcazaciuc
Copy link

@saboteurspk - I maintain the above extension. Currently, as far as i know, none of the flow extensions for VSCode offer providers for find all references, go to definition etc. It's the Typescript service built into VSCode that provides them and currently doesn't work so well it seems.
I'll begin soon adding support based on Flow server for these features.

@ghost
Copy link

ghost commented Dec 12, 2016

Yes, find all references etc. doesn't work well in all others, but in that other extension it was much more broken than in this one :) The only reason I have chosen that was that I was able to debug it, even if I like babel projects much more.
Nice to hear about planned improvements. Will do an beta tester at least :)

@orta
Copy link
Contributor

orta commented Dec 16, 2016

@saboteurspk is there any reason you won't contribute that work to this extension? Having multiple competing extensions trying to do the same thing seems like a waste of all of our time

@faceyspacey
Copy link

+1

the use of Flow for React is severely limited if it only points out errors in the component itself with regards to missing props, not its usages.

When something like 80% of most react apps are made up of react components, and basically the most common way to use components is with optional props, that means you can't be informed of static errors where they are happening in the majority of the app. That makes flow basically unusable for React with VSCode.

@orta
Copy link
Contributor

orta commented Jan 16, 2017

VSCode error diagnostics are limited to only one single line of code, for us to handle error codes in the way that works for how Flow outputs them we'd need microsoft/vscode#1927 to be built out.

@faceyspacey
Copy link

faceyspacey commented Jan 17, 2017

@orta and until then, we can't pre-parse the lines and just report the one on the usage, which is more important?

Also, I find it hard to believe we can't parse the flow errors into 2 errors without needing to make it a single multi-line error.

@orta
Copy link
Contributor

orta commented Jan 17, 2017

You're welcome to give it a shot 👍

Flow + vscode works well in all the projects that I've been using it in ( jest, react-native and the occasional react+relay app ) and so if it's not working in your apps, I'd love to see improvements to this extension

@faceyspacey
Copy link

faceyspacey commented Jan 18, 2017

Wow, and I'm just trying to get through one app this year.

...I spent basically all of yesterday finding all the issues in both Atom and VS Code. My findings are:

  • the most popular Flow package for Atom is way better and solves these problems.
  • The reporting engine produces 2 errors: one that links to the definition and one to the usage. And there are many other cases where this is a problem rather than just undefined react props.
  • With more complex types--like those you might find made for react-redux (https://github.com/reactjs/redux/blob/master/flow-typed/react-redux.js)--more complex issues appear that require multi-line errors or multiple errors.
  • The Eslint implementation is also way better--it links back to the github readmes for each type of issue. And in general, the error rows are way more readable--they are broken up into colored buttons, rather than just a string per row, with each button possibly linking to a different place. The packages used are linter-flow and linter-eslint, both by AtomLinter. And in the case of eslint, plugin:flowtype/recommended is also used for additional eslint reporting based on flow's static types.

It's a MAJOR difference between VS Code where basically every error only links back to the definition. You can't really do much with that. You basically have no idea which usage in your codebase is causing the problem, especially if you didn't just write the code. In comparison, it's useless. I'm moving back to Atom for this sole reason. Well, I'm gonna keep both editors open for the time being--VS Code for debugging.

That said besides this, I think VS Code is better. So as far as improving this in VS Code, do you know of the relevant lines of code? Can you link them. It sounds like it very well may be something we can do here without depending on the VS Code developers to provide multi-line support. Like I said, why do we need that--just generate 2 errors. The work seems like it should be all on the Flow side of things from an outsider's perspective.

@orta
Copy link
Contributor

orta commented Feb 10, 2017

Facebook use Atom, and so there are people who can and do make sure Flow + Atom is 💯 for Nuclide.

On this VS Code side though, it is a flow devs + community effort. I've been trying to help out a bit, as I used to have a lot of work JS + Flow code. It's very reasonable that it's not as polished alas.

WRT fixing some of these issues, a lot of it comes from the mapping between the nuclide flow codebase and then the VS Code extension. I'd expect the work for the issue would be inside this function - notably the "details section".

One thing that might get tricky is that the order of your errors would be determined by the line of code and the filename, and so I'd maybe consider fudging it and just placing subsequent issues on the line under the main, and add line messages? Unsure how that'd feel

@kumarharsh
Copy link

@orta VSCode is going to have an experimental API for multiple error/warning diagnostics (microsoft/vscode#10271). Can you take a look at it and provide any inputs specific to flow which would make it more suitable to show these kind of errors there? I could provide them myself, but I don't work on linux and flow-for-vscode hangs my system frequently, and thus have fallen behind on a lot of recent updates/developments on Flow side of things.

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 a pull request may close this issue.

7 participants