Use the correct language ID for JavaScript & TypeScript

[hovsater]

The TypeScript language server expects the languageId to be javascript for .js, javascriptreact for .jsx, typescript for .ts, and typescriptreact for .tsx. In practice, only javascript, typescript and typescriptreact are relevant.

Since the goal is to keep language scopes inline with what other editors are doing, I didn't want to change the scope for tsx to source.typescriptreact.

Instead, I changed the implementation so that the language name is used as the languageId for LSP. This works, since we can set tree-sitter-library explicitly if the language name doesn't match the name of the tree sitter library.

While this doesn't seem to break any of the existing languages, I'm not sure if this is fine, or if we should introduce a new configuration option for languages called language-id and only fallback to using the language name if it's not set.

Any feedback is much appreciated. 🙂

[archseer]

I don't quite like this since now our language names are tied to what the LSP expects (which might also be different per language server). Let's instead add a language-id key to https://github.com/helix-editor/helix/blob/01782ee16adcffd56af44890ad772477dbefe610/helix-core/src/syntax.rs#L93-98

pub language_id: Option<String>

Then the lsp code can use config.language_server.language_id.unwrap_or(config.name)

[hovsater]

Yeah, I thought so as well (I briefly mentioned it in the pull request description). I'll update the PR. 🙂

[hovsater]

Hmm, should it really be part of the LSP settings and not the language itself? It's not really a configuration of the LSP, but more specifically an identifier of the language itself. I think it would make more sense to put language-id under the language section. Then language-id could default to the language name if language-id wasn't present.

Thoughts?

[archseer]

It's a lsp-specific language identifier. There's no standard conventions here, so I'd prefer to keep it grouped under the language server. It's quite likely that there could be another Javascript language server that expects a different identifier

[hovsater]

Fixed in c42377c.

[archseer]

Let's use map.or_else(|| ...) to avoid computing the fallback unless it's needed. Also rsplit_once is a nice optimization over split(..).last():

Suggested change

	let fallback = self.language().and_then(|s| s.split('.').last());
	self.language
	.as_ref()
	.and_then(|config| config.language_server.as_ref())
	.and_then(|lsp_config| lsp_config.language_id.as_ref())
	.map_or(fallback, |id| Some(id.as_str()))
	self.language
	.as_ref()
	.and_then(|config| config.language_server.as_ref())
	.and_then(|lsp_config| lsp_config.language_id.as_ref())
	.map_or_else(
	|| self.language().and_then(|s| s.rsplit_once('.').map(|(_, last)| last)),
	|id| Some(id.as_str())
	)

I'm also wondering why not

Suggested change

	let fallback = self.language().and_then(|s| s.split('.').last());
	self.language
	.as_ref()
	.and_then(|config| config.language_server.as_ref())
	.and_then(|lsp_config| lsp_config.language_id.as_ref())
	.map_or(fallback, |id| Some(id.as_str()))
	self.language
	.as_ref()
	.and_then(|config| config.language_server.as_ref())
	.map(|lsp_config| lsp_config.language_id.as_str())
	.unwrap_or_else(|| self.language().and_then(|s| s.rsplit_once('.').map(|(_, last)| last)))

[hovsater]

Fixed in cc80d3e.

I'm also wondering why not

I wasn't allowed to call as_str() on std::option::Option. On a more general note, I'm still very new to Rust, so my code is probably less than ideal most of the time. Happy to learn from feedback though. 🙂

[pickfire]

Missing / on second line?

[hovsater]

Fixed in 2e0d66c.

[archseer]

Looks good now, thanks :) 🎉