search

xojs / vscode-linter-xo

Linter for XO
MIT License
176 stars 29 forks source link

Errors and skips linting outside of a workspace #108

Closed sholladay closed 2 years ago

sholladay commented 2 years ago

Hi. First, thanks for all of the recent work on this extension. It used to be quite buggy, but seems like it's improved a lot in the last year.

I've got everything working correctly where if I run code . or code /path/to/folder in my terminal, VS Code opens and the extension successfully lints the code. All settings are at their default and settings.json is empty. XO is installed locally and configured via package.json.

However, if I instead run code index.js in my terminal, the following errors are shown in the VS Code output tab:

[Info  - 12:25:46 AM] XO Server Starting in Node v14.16.0
(node:18145) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'map' of null
    at Sc.handleDidChangeConfiguration (/Users/sholladay/.vscode/extensions/samverschueren.linter-xo-3.7.0/dist/server.js:46:5063)
    at zt (/Users/sholladay/.vscode/extensions/samverschueren.linter-xo-3.7.0/dist/server.js:4:1434)
    at lr (/Users/sholladay/.vscode/extensions/samverschueren.linter-xo-3.7.0/dist/server.js:3:11164)
    at Immediate.<anonymous> (/Users/sholladay/.vscode/extensions/samverschueren.linter-xo-3.7.0/dist/server.js:3:11046)
    at processImmediate (internal/timers.js:461:21)
(Use `Code Helper (Renderer) --trace-warnings ...` to show where the warning was created)
(node:18145) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:18145) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
[Error - 12:25:46 AM] Cannot read property 'uri' of undefined
[Error - 12:25:46 AM] No valid workspace folder could be found for this file. Skipping linting as it is an external JS file.

The second "cannot read property" error doesn't have a stack trace for some reason (probably another bug of sorts?) but I guess it must be coming from this line: https://github.com/xojs/vscode-linter-xo/blob/81094e68829f1e0c3cf00c04b914c9fe5e51a27b/client/extension.js#L94

As I understand it, the only real difference between code index.js and code . followed by opening index.js is that only the latter opens a "workspace". Seems silly to me, like VS Code should be smarter and find the project root, or even just use the parent directory. Maybe I'm the edge case and everyone else is opening folders? Anyway, I tend to use tab completion in the terminal to open a file directly instead of drilling down through the folder structure in the GUI. So fixing support for linting without a workspace would be a big help for me.

Environment: Extension version: 3.7.0 VS Code version: 1.62.0 OS: macOS 12.0.1

Probably doesn't matter here, but I'm on the new 2021 MacBook Pro with the M1 Max chip, so everything is compiled for ARM. Both node and VS Code were installed via Homebrew, with some things being laid out on disk a little differently compared to Intel x86 Macs.

spence-s commented 2 years ago

Hi. First, thanks for all of the recent work on this extension. It used to be quite buggy, but seems like it's improved a lot in the last year.

Thank you!

@sholladay - unfortunately the entire architecture of the extension right now is based around workspace folders. It does look like there are a few unhandled errors around opening single files that I need to take care of. However, we purposefully do not lint them by design. I made this decision for both simplicity and performance.

We may fully support this in the future, but I don't think this will come for a long while as we have quite a few more complex features to get in and I don't think this is a common workflow.

In order to fully support opening one-off files, we have to cache all the data on a per file basis as we have no way of determining where the individual file is coming from so it will require a lot more logic around looking up where the file is being opened from that vscode otherwise takes care of for us (being folder based). That will introduce a lot of overhead and a lot more edge cases to handle.

I'm not completely opposed to doing this eventually, but it's a very low priority thing with a lot of work and testing to make the change.

In the meantime, I would propose maybe a small change to your workflow, in that you if you open up the workspace folder first, you can still use the command line to open individual files as you please and code is smart enough to open the file in an already opened workspace. It may be worth having a single vscode multiroot workspace with all your common projects in it side by side that generally stays open to make this seamless for everything you do. Hope this is an acceptable work around.

Finally - if you'd like to tackle this change, I'd be happy to show you the relevant code and changes that need to happen!

Going to leave this issue open for visibility for a short time and see if others want support for this or feel this is an important feature. Personally, I am most always working in a folder.

sholladay commented 2 years ago

unfortunately the entire architecture of the extension right now is based around workspace folders.

Okay, thanks for the clarification. I was afraid of that. The docs aren't entirely clear on this point. They do say XO must be installed locally in your workspace folder, which I suppose could be interpreted to mean that a workspace is required, but I guess partly because VS Code's workspace semantics are almost invisible and partly because earlier versions of this extension were the exact opposite (only worked without a workspace), I figured this was supposed to work, even after seeing the error output. It gets more unclear when you realize the statement about requiring a local install isn't exactly correct, as the docs then go on to talk about using a global install and linting Deno code.

In order to fully support opening one-off files, we have to cache all the data on a per file basis as we have no way of determining where the individual file is coming from so it will require a lot more logic around looking up where the file is being opened from that vscode otherwise takes care of for us (being folder based). That will introduce a lot of overhead and a lot more edge cases to handle.

By determining where the file "is coming from", do you mean the working directory? Is VS Code only providing you with a relative path and not the full path when I open an individual file? That would seem very odd to me, but certainly understandable if that's the case.

This seems like one of those situations where there's a certain logic to the quirky behavior for those of us that understand it, but less technical users will get utterly lost.

In the meantime, I would propose maybe a small change to your workflow, in that you if you open up the workspace folder first, you can still use the command line to open individual files as you please and code is smart enough to open the file in an already opened workspace.

That's an interesting tip. I'll try that and it sounds workable so long as I can also open other ancestor or sibling files and workspaces in the same VS Code window and not have it do anything too weird.

Finally - if you'd like to tackle this change, I'd be happy to show you the relevant code and changes that need to happen!

Sure, at the very least I'll certainly help with the docs and I'll also poke around the VS Code API a bit to see if I can find relevant context variables that might help here. Might take me a while to get around to it, but if I find something, I'll send a PR.

spence-s commented 2 years ago

@sholladay - I got curious about what this would take and I came up with a really simple way to make it work pretty reliably. Try the latest version (3.8). If you open a file now and you don't have a workspace open, we find the nearest package.json and use that as the workspace. I thought this would be way more complex 😆 . Happy hacking.

Let me know if you have any trouble.

sholladay commented 2 years ago

Happy to confirm that 3.8 works for me. Thanks. :)

Will obviously need a different solution for Deno. I'm looking forward to trying this out on Pogo.

spence-s commented 2 years ago

Happy to confirm that 3.8 works for me. Thanks. :)

Awesome!

Will obviously need a different solution for Deno. I'm looking forward to trying this out on Pogo.

The best option that will fit in your workflow (opening all files as one offs) and any others I think would be to just put a package.json at the project root and install xo locally in the project via npm. I think using npm for some dev deps is still pretty common in deno projects, no? If you didn't want to include it in your project you could add pogo/node_modules and pogo/package.json to your user .gitignore.

Another option (which will require a more conventional workflow and open the project directory first) is to add a "xo.path" option to your workspace settings that points to a global (or any other) install of xo. Optionally ignore .vscode folders in your user .gitignore.