Skip to content
This repository was archived by the owner on Dec 19, 2024. It is now read-only.

Refactor diagnostics for perf#126

Merged
orta merged 6 commits intoflow:masterfrom
thymikee:refactor-diagnostics
May 17, 2017
Merged

Refactor diagnostics for perf#126
orta merged 6 commits intoflow:masterfrom
thymikee:refactor-diagnostics

Conversation

@thymikee
Copy link
Copy Markdown
Contributor

@thymikee thymikee commented May 11, 2017

Summary

  • Fixed hasFlowPragma check regex
  • Run flow only if @flow pragma is detected
  • Debounce all requests to flow with 500ms timeout (matches eslint behavior)
  • Run prettier through flowDiagnostics
  • Delete .flowconfig from playground, as it unabled flow check (not sure why though)
  • Add yarn.lock

Overall this should drastically reduce number of requests to flow server when runOnEdit is enabled.

Fixes #116
Fixes #115

cc @orta

Copy link
Copy Markdown
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Looks good to me - thanks @thymikee

Comment thread lib/flowDiagnostics.js
const isDocumentActive = activeTextEditor.document === event.document;

if (isDocumentActive && isRunOnEditEnabled() && hasFlowPragma(event.document.getText())) {
debouncedUpdateDiagnostics(context, event.document);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice touch 👍

Comment thread .vscode/settings.json Outdated
"files.exclude": {
// "build": true, // set this to true to hide the "build" folder with the compiled JS files,
"npm-debug.log**": true
"npm-debug.log": true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would show the logs I think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, changed to single star

Comment thread lib/utils/util.js
/^\s*\/\*+\s*@flow\s*\*+\//m.test(content) ||
/^\s*\/\/\s*@flow\s*$/m.test(content)
return hasPragma
return /^\s*(\/*\*+|\/\/)\s*@flow/m.test(content);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Gozala do you know if we're going to miss out of things but this switch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previous regex didn't catch things like

/*
 * @flow
 */

which are pretty common.
I tried to cover most cases out there: http://regexr.com/3fumm

We could further optimise checking for pragma on larger files while typing, by caching the result and revalidating if the pragma is still there once in a while.

@svipas
Copy link
Copy Markdown

svipas commented May 13, 2017

@orta When extension will be updated with this PR?

@orta
Copy link
Copy Markdown
Contributor

orta commented May 17, 2017

OK, I got half an hour - gonna give this a local clone and a run through myself then if it's 👍 I'll merge and prepare for the next release.

@orta
Copy link
Copy Markdown
Contributor

orta commented May 17, 2017

Looking good 👍

screen shot 2017-05-17 at 21 06 17

This extension is definitely not cleanup up after itself, but I don't think that's the responsibility of this PR, as it's been known for a long time. Let's get this in and published.

@orta orta merged commit f8e3f00 into flow:master May 17, 2017
@thymikee thymikee deleted the refactor-diagnostics branch May 17, 2017 20:09
@orta
Copy link
Copy Markdown
Contributor

orta commented May 17, 2017

Bah, this repo needs Danger - no CHANGELOG :D

@thymikee
Copy link
Copy Markdown
Contributor Author

It need quite a lot of goodies, like tests for example XD

@orta
Copy link
Copy Markdown
Contributor

orta commented May 17, 2017

Hah - yeah 📟

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants