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

Only invoke flow after hyperclick word is clicked #105

Open
steelbrain opened this issue Jul 26, 2017 · 6 comments
Open

Only invoke flow after hyperclick word is clicked #105

steelbrain opened this issue Jul 26, 2017 · 6 comments

Comments

@steelbrain
Copy link
Owner

We could use Atom APIs to get word boundary at a range and by default show it as clickable, and after the user clicks it we can invoke Flow to jump for us

Fixes #103

Thoughts @leos @lloiser?

@leos
Copy link
Contributor

leos commented Jul 26, 2017

I'm loosely against that. I don't know if flow's word boundaries are the same as Atom's - it also doesn't give you the underlining feedback of whether something is clickable.

Let me look at #103 - I have a few thoughts.

@leos
Copy link
Contributor

leos commented Jul 26, 2017

@steelbrain - My initial theory is that https://github.com/steelbrain/flow-ide/blob/master/lib/index.js#L58 won't find a flow.exe file, which triggers a more expensive invocation of flow that goes through node.exe. I need to dust off my windows machine to test that, doing it now.

@leos
Copy link
Contributor

leos commented Jul 26, 2017

Confirmed. flow.exe is lightning fast, invoking it via node.exe can take a second. @steelbrain can you reopen #103 and close this? Or create a new issue to track this work if you don't want to continue the reporting issue?

@lloiser
Copy link
Collaborator

lloiser commented Jul 31, 2017

I have found a lot of places where flow fails to go to the definition. Marking these spots as clickable would then lead to issues in our repo again.
I think we have to further investigate the actual slowdown when we call flow. It seems that @leos already found something. I will test it as soon as I can.

@leos
Copy link
Contributor

leos commented Jul 31, 2017

Here's the issue that got filed with flow - facebook/flow#4445 - they haven't responded (possibly because it's closed) so maybe open up a new one with the same question?

@leos
Copy link
Contributor

leos commented Jul 31, 2017

Pushing the PR that changes search path order should allow folks to workaround the issue by invoking flow.exe explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants