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 package.json and (and tsconfig.json) for TS/JS language config roots #10652

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jpaju
Copy link
Contributor

@jpaju jpaju commented May 1, 2024

No description provided.

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels May 2, 2024
@archseer
Copy link
Member

archseer commented May 6, 2024

Hmm won't this break package workspaces though? Usually you want the top level package.json/git root there

@jpaju
Copy link
Contributor Author

jpaju commented May 6, 2024

Yes, that's probably true. Currently folder structure for monorepos below doesn't work properly since Helix uses folder with .git folder as root:
|-- .git/
|-- service1/
| ---- package.json
|-- service2/
| ---- package.json

Not sure we could make them both (workspaces and structure below) work?

@archseer
Copy link
Member

archseer commented May 6, 2024

See #5748

@the-mikedavis
Copy link
Member

the-mikedavis commented May 7, 2024

I believe we use the topmost directory with a root marker:

helix/helix-lsp/src/lib.rs

Lines 1009 to 1066 in beb5afc

/// Find an LSP workspace of a file using the following mechanism:
/// * if the file is outside `workspace` return `None`
/// * start at `file` and search the file tree upward
/// * stop the search at the first `root_dirs` entry that contains `file`
/// * if no `root_dirs` matches `file` stop at workspace
/// * Returns the top most directory that contains a `root_marker`
/// * If no root marker and we stopped at a `root_dirs` entry, return the directory we stopped at
/// * If we stopped at `workspace` instead and `workspace_is_cwd == false` return `None`
/// * If we stopped at `workspace` instead and `workspace_is_cwd == true` return `workspace`
pub fn find_lsp_workspace(
file: &str,
root_markers: &[String],
root_dirs: &[PathBuf],
workspace: &Path,
workspace_is_cwd: bool,
) -> Option<PathBuf> {
let file = std::path::Path::new(file);
let mut file = if file.is_absolute() {
file.to_path_buf()
} else {
let current_dir = helix_stdx::env::current_working_dir();
current_dir.join(file)
};
file = path::normalize(&file);
if !file.starts_with(workspace) {
return None;
}
let mut top_marker = None;
for ancestor in file.ancestors() {
if root_markers
.iter()
.any(|marker| ancestor.join(marker).exists())
{
top_marker = Some(ancestor);
}
if root_dirs
.iter()
.any(|root_dir| path::normalize(workspace.join(root_dir)) == ancestor)
{
// if the worskapce is the cwd do not search any higher for workspaces
// but specify
return Some(top_marker.unwrap_or(workspace).to_owned());
}
if ancestor == workspace {
// if the workspace is the CWD, let the LSP decide what the workspace
// is
return top_marker
.or_else(|| (!workspace_is_cwd).then_some(workspace))
.map(Path::to_owned);
}
}
debug_assert!(false, "workspace must be an ancestor of <file>");
None
}

so if we a workspace like

.
+-- package.json
`-- packages
   +-- a
   |   `-- package.json
   `-- b
       `-- package.json

we should use that top-most package.json. I believe we have similar roots set up for Elixir for example with mix.exs or mix.lock that should still work inside umbrella workspaces

@jpaju
Copy link
Contributor Author

jpaju commented May 9, 2024

I tried this out and can confirm that the behavior is what @the-mikedavis described! When package.json is specified as roots in languages.toml, Helix reports the correct directory in rootUri/rootPath/workspaceFolders. This applies both to the monorepo folder structure I initially described and to workspace folder structure that @archseer described that has a top-level package.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants