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

The language server cwd is not a good fallback for unified-engine cwd #29

Closed
4 tasks done
remcohaszing opened this issue Jan 30, 2022 · 7 comments · Fixed by #33
Closed
4 tasks done

The language server cwd is not a good fallback for unified-engine cwd #29

remcohaszing opened this issue Jan 30, 2022 · 7 comments · Fixed by #33
Assignees
Labels
💪 phase/solved Post is done

Comments

@remcohaszing
Copy link
Member

Initial checklist

Affected packages and versions

2.x

Link to runnable example

remarkjs/vscode-remark#65

Steps to reproduce

  • git clone git@github.com:remcohaszing/vscode-remark.git
    cd vscode-remark
    npm ci
  • Close all instances of Visual Studio Code
  • Open Visual Studio Code either in Windows or from any directory that isn’t the repo you just cloned
  • Start debugging
  • In the debugged VS Code instance which opened readme.md, add the text Foo. Bar.

Expected behavior

A diagnostic should appear

Actual behavior

No diagnostic appears.

Further inspection showed this is caused by the fact that the current working directory of the language server doesn’t reflect the file opened.

I found this out first by troubleshooting Windows. This shows the following values:

  • workspacesAsPaths: C:\Users\remco\AppData\Programs\Microsoft VS Code
  • file.path: z:\vs-code\readme.md

There is no correlation between the two.

I was also able to reproduce this on Linux by opening VSCode using the desktop launcher. This sets the cwd to /home/remco, while the file is in /home/remco/Projects/vscode-remark/readme.md. remark should be resolved from /home/remco/Projects/vscode-remark, not /home/remco.

While typing this issue, I also noticed the different casing styles used for the drive letters on Windows.

Runtime

Node v16

Package manager

npm v7

OS

Other (please specify in steps to reproduce)

Build and bundle tools

esbuild

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jan 30, 2022
@remcohaszing
Copy link
Member Author

Some possible solutions:

  1. Traverse the parent directories until a package.json file is found and resolve from that directory.
  2. Traverse the parent directories until a VCS directory is found and resolve from that directory.
  3. Traverse the parent directories until a remark configuration is found and resolve from that directory.
  4. Resolve from the parent directory directory of the checked file.

@remcohaszing
Copy link
Member Author

I personally like option 3. This also aligns with my proposed solution for #30.

@wooorm
Copy link
Member

wooorm commented Jan 31, 2022

  1. I believe this to be the correct solution.
  2. is incorrect in my opinion because this project does not depends on VS Code.
  3. is incorrect because configuration files can be deeper and higher that “working directories”. I also think that config files are optional.
  4. is incorrect in my opinion as it would break existing plugins.

@remcohaszing
Copy link
Member Author

  1. I believe this to be the correct solution.

This is fine with me. I’ll go for this option then.

  1. is incorrect in my opinion because this project does not depends on VS Code.

I wrote VCS (version control system), not VSC (VS Code). Anyway, I don’t think this is the ideal option.

  1. is incorrect because configuration files can be deeper and higher that “working directories”. I also think that config files are optional.

package.json files can also be higher up the directory tree due to mono repos. Anyway, I’ll go for option 1.

@ChristianMurphy
Copy link
Member

Traverse the parent directories until a package.json file is found and resolve from that directory.

🤔 what about npm/yarn workspaces?
It would resolve to the child package.json, but would we actually want the parent one?

Traverse the parent directories until a VCS directory is found and resolve from that directory.
(version control system),

Going to the git or svn root could be a good option as well 🤔
Though determining root could get interesting if sub-modules are involved.

is incorrect because configuration files can be deeper and higher that “working directories”. I also think that config files are optional.

It's also worth noting eslint handles this by continuing to scan up until root/home folder, or until a configuration with "root": true.
While it doesn't exist today, adding a root option to unified engine could also be an option.

@wooorm
Copy link
Member

wooorm commented Feb 2, 2022

I wrote VCS (version control system), not VSC (VS Code). Anyway, I don’t think this is the ideal option.

I’m also open to both package.jsons and .git. That would help users that don’t want to install remark and friends locally but are using Git.

what about npm/yarn workspaces?
It would resolve to the child package.json, but would we actually want the parent one?

Going to the git or svn root could be a good option as well 🤔
Though determining root could get interesting if sub-modules are involved.

I think it’s fine to find subpackages/submodules.

It's also worth noting eslint handles this by continuing to scan up until root/home folder, or until a configuration with "root": true.
While it doesn't exist today, adding a root option to unified engine could also be an option.

This issue is about figuring out, from a file, what “working directory” it belongs to.
I see that as unrelated to how to find configuration and how to merge it.
Node programs sometimes use working directories but Electron doesn’t define them.
Configuration files can be shared (as in node_modules/), placed in different folders (config/), they can be missing, there can be multiple per “workspace”.

@github-actions

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Mar 4, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

3 participants