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

Add flow #52

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add flow #52

wants to merge 3 commits into from

Conversation

Will-Sommers
Copy link
Contributor

Heyo @pokey

It looks like the tsx parser supports flow types. The flow and hack additions were previously one but I separated them because I think there should be some discussion here.

I installed this Flow Language Support extension to see if it defined a languageId is defined using the plugin. I'm not seeing the ability to create Flow files which makes me think that I'm either doing something wrong or it might break the languageId->parser default mapping.

I'm going to play around with it a bit more and see what's up.

@pokey pokey marked this pull request as draft October 10, 2022 10:56
@Will-Sommers
Copy link
Contributor Author

Heyo @pokey — circling back on this. Although the PRs here are a little dated, I'm going to restart the conversation. Most people seem in favor although I guess it will likely break other plugins that rely on JS being defined.

michaelgmcd/vscode-language-babel#95
flow/flow-for-vscode#187

If these changes didn't happen, I guess I could look at building a custom extension which shouldn't be a huge huge deal to keep updated.

Other options don't seem great since languageId is so tied to which grammar is chosen. I think we'd need to get into some sort of override/configuration via CSV or something like that. There's a lot that would go into that including bundling a parser here that is never used by default, so I'll leave it here for further pondering.

@pokey
Copy link
Member

pokey commented Oct 12, 2022

I'm not sure I understand the problem from Cursorless / parse tree perspective. If the tsx parser handles flow out of the box, why can't we just merge this, and then do roughly the same thing in cursorless, like we do for sass / css? And then if necessary we can add another two lines to activate on flow language id as well

But maybe I'm missing something?

@Will-Sommers
Copy link
Contributor Author

Will-Sommers commented Oct 24, 2022

Right now the public extension doesn't expose a new languageId, unlike the Sass/css.

I'm not sure how cursorless will determine which parser to use given this.

@pokey
Copy link
Member

pokey commented Oct 24, 2022

When you say "which parser to use", I'm confused. You mentioned earlier that the tree-sitter-tsx parser supports flow out of the box. So what are the parsers to choose from here? Is it not just one parser? Do you mean choosing between typescript and tsx parser? Or by "parser" do you mean which Cursorless patterns should be active? Maybe it's worth dropping into a meet-up to discuss; there's one tomorrow

But if not super high priority on your end no rush

@Will-Sommers
Copy link
Contributor Author

Thanks for the quick response @pokey. Let me ask a question instead of making an assumption.

If there is now languageId for flowjs files defined, will we have a problem? Right now the public flow extension treats flow files as regular javascript files and doesn't define a languageId for htem.

@pokey
Copy link
Member

pokey commented Oct 24, 2022

If that's the case, and if the parser we use for javascript files already supports flow, then you should be able to just add any necessary patterns to our typescript patterns in cursorless and then you don't need this PR

@pokey
Copy link
Member

pokey commented Jan 13, 2023

sorry I lost track of this one. Where did we land on this and #51?

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 this pull request may close these issues.

2 participants