Handle null values in linters wrapped by the LinterAdapter #906
Conversation
|
@hansonw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
In steelbrain/linter, what happens when a provider returns As implemented, this PR means that if a provider returns some results, then on a subsequent call returns |
|
@nmote The linter package does what you mentioned in its implementation here: https://github.com/steelbrain/linter/blob/v1/lib/linter-registry.js#L65 so I think that's reasonable. I'd imagine you would return empty to invalidate the current messages. |
|
Perfect, then 👍 |
| } | ||
|
|
||
| const result = await this._requestSerializer.run(maybe); | ||
| if (!result) { |
There was a problem hiding this comment.
RequestSerializer::run never returns null or undefined. If the underlying promise resolves to null it will return an object with a null result field.
Instead you'll need to check linterMessages (below).
This change should also change the types in nuclide-diagnostics-common/lib/main.js to reflect the actual lint return type. Then Flow should guide you through the necessary changes.
|
Thanks for your response guys. Before I continue, could you shed some light on these errors I get running Are they "by design", or did I miss a build step? I don't get any errors related to |
|
That's our fault, and frankly it's something I'm really unhappy about. We have various places where for the sake of expedience we've added some facebook-internal functionality into these I don't have a great solution for you, to be honest. Probably just run Flow yourself and ignore these errors. And for running tests, just run the tests for the package you are working on. Fixing this properly will take a concerted effort on our part. A lot of us want to do it but we just haven't had the time. In the meantime if you have ideas for how to ease the pain I would be more than happy to review pull requests (maybe adding options to the script or something). Feel free to open issues to discuss ideas and CC me so I get to them promptly. |
|
For the time being a lot of these fb-only requires are preceded by a |
|
But, I don't think we've been diligent about adding those comments so it might not get all of them. And be careful not to include the config change in your PR :/ |
* https://github.com/facebook/nuclide: (106 commits) Consolidate text-buffer utils Rename buffer.js -> text-buffer.js Revert Confirmation: Ask for confirmation before reverting from file changes lists Transform buck test failures into diagnostics Throw with a descriptive error when a circular transpile is found Do not use maybeToString in nuclide-logging's stacktrace Move to Workspace View Task Runner: further reduce unnecessary re-renders require language-graphql package to be installed for using as an activation hook fix the font generator to set lower baseline && normalize the icon sizes add GraphQL logo to nuclicons Create throttle operator Navigation Stack in the FAQ and keyboard shortcuts Add uncommitted changes to source control sidebar Ask for confirmation before reverting a file Add highlight when dragging over textareas Actually add file to Mercurial rather than revert Add `shorten` to node-commons s/trackOperationTiming/trackTiming/g Fix insertText return value ...
|
@oleander updated the pull request - view changes - changes since last import |
|
@nmote Now everything passed. |
| editor.getPath(), | ||
| linterMessages, this._provider.providerName || this._provider.name, | ||
| ); | ||
| if (!this._enabled) { |
There was a problem hiding this comment.
Good call with the early returns here. It reads much better.
| return; | ||
| } | ||
|
|
||
| if (this._provider.invalidateOnClose && !this._onDestroyDisposables.has(buffer)) { |
There was a problem hiding this comment.
Any particular reason why this block happens before the linterMessages null check? Or did you just choose the order arbitrarily?
There was a problem hiding this comment.
Tried to make as small changes as possible until the spec passed. The linterMessages = result.result row was placed below the block so kept it there, just in case. Do you want me to move it?
The code would look more symmetrical if the guard was moved above the block, so I don't mind moving it.
There was a problem hiding this comment.
I don't really care either way. I don't think it's important. I just wanted to make sure there wasn't some subtlety I was overlooking.
| } | ||
|
|
||
| const maybe = this._provider.lint(editor); | ||
| if (!maybe) { |
There was a problem hiding this comment.
We prefer to do null/undefined checks explicitly: if (maybe == null). Better to write what you actually mean even though it's a few more characters, rather than relying on truthiness which has some odd quirks.
There was a problem hiding this comment.
Aha, okay. I'm so used to ESLint complaining about not using === so almost forgot about == null being the same as === undefined || === null.
There was a problem hiding this comment.
Yeah, that's the one exception we make in our config. It's basically due to Flow's ?something being shorthand for something | null | undefined. Since it's used so pervasively we basically have to conflate null and undefined. Oh well.
|
@oleander updated the pull request - view changes - changes since last import |
|
Fixed all bugs™ |
|
Thanks, LGTM |
Summary: Add notice about the `$FlowFB` tag used in flow for non Facebook employees (see: #906 (comment)) to avoid errors similar to those listed here: #906 (comment). This also fixes tree flow errors related to missing, misspelled or unused `$FlowFB` tags. Ping hansonw. Closes #912 Differential Revision: D4261281 Pulled By: nmote fbshipit-source-id: 987646425485c8734311c117fcf69a45cd6dde2c
The linter provided by steelbrain/linter allows for
nullvalues to be returned by a provider, while the current LinterAdapter wrapping it, doesn't. Here's the relevant code in steelbrain/linter.The linter-eslint package is currently returning
null(pull request/discussion, current implementation).Example usage:
The current implementation of Nuclide throws the following error on
nullwhen using the latest version oflinter-eslint.This PR aims to solve this by checking the return value of a linter.