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 JS/TS code folding support to language server #632

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

Conversation

camerondubas
Copy link
Contributor

@camerondubas camerondubas commented Oct 14, 2023

Part of #626

This adds support for code folding in js & ts files. This implementation is based on tsserver's. This does not add support for gts/gjs files.

Folding seems to work as expected, but two odd things had to be handled:

  • The OutliningSpans have TextSpans with inclusive length, leading to off-by-one errors. I handled this inline, but I'm concerned that this is unintentional and may not always be the case.
  • There are some additional OutliningSpans that incorrectly have a "0" starting offset and don't match any foldable code. I believe these are retrieved from the template file and are meant to represent their folding ranges. As is, these are superfluous and cause an incorrect FoldingRange on line 1 of every component file.

Any guidance on better ways to handle either of these would be appreciated!

Recording.2023-10-14.233242.mp4

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Oct 17, 2023

Choose a reason for hiding this comment

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

I really appreciate you adding all these tests in your PRs <3

expect(folds).toEqual([
{
startLine: 0,
endLine: 6,
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Oct 17, 2023

Choose a reason for hiding this comment

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

is this excluding the last }? I count that it should be 7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last line is excluded if it's just a closing bracket (https://github.com/typed-ember/glint/pull/632/files#diff-cadffc59ac4fd3c9fd1cd7488980fc9cfea40cd5a93129f1021c54c810980e20R503). This is so that folded functions keep the } visible:

// Like this when folded
function topLevel() {
}

// instead of this
function topLevel() {

this is consistent with the TS language server's folding.

@dfreeman
Copy link
Member

Thank you @camerondubas!

I just wanted to say this hasn't been forgotten about; you've asked some good questions about the implementation that I'm expecting will take a little time to investigate, and I haven't had a lot of uninterrupted time this week. I'm aiming to give a more thorough look early next week, but please ping me if you don't hear anything—I want to make sure this doesn't sit idle too long!

@camerondubas
Copy link
Contributor Author

@dfreeman I updated this with the latest main. Friendly bump to take a look at this when you have some time. No rush though, life's busy! Cheers

@lifeart
Copy link

lifeart commented Dec 15, 2023

@NullVoxPopuli
Copy link
Contributor

@camerondubas any chance you want to rebase? <3

@NullVoxPopuli
Copy link
Contributor

looks like a floating dep dropped node 16, breaking our ci.

I'd consider this non-blocking for merge, myself, and fixing the floating dep issue can happen in a different PR

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.

4 participants