xojs / vscode-linter-xo

Linter for XO
MIT License
176 stars 29 forks source link

import/order order has different behavior across node versions - vscode forces xo to run in node v14 #103

Closed novemberborn closed 3 years ago

novemberborn commented 3 years ago

https://github.com/avajs/ava customizes the import ordering so that it deviates from XO 0.44. When I format AVA code using this extension, the imports are reordered according to the XO rules.

I haven't noticed any other rules being disregarded like this, but that may be due to the files I've been editing.

spence-s commented 3 years ago

This extensions does not do anything but apply the formatting directly from running the unaltered text of the file through xo. There are many other plugins and settings in VSCode that can reorder imports.

Can you verify that running xo path/to/test.js --fix has a different output than the extension and maybe expand a bit on why you think the extension is the issue?

spence-s commented 3 years ago

@novemberborn I tested on ava - and xo from cli is giving me same results as extension, and it looks to me like it's respecting the rules set up in the .xo-config.json.

novemberborn commented 3 years ago

All I know is that it's a problem when I enable the extension.

It doesn't make sense why this would be the only rule that's disregarded though.

Perhaps there's something else VSCode does that is applied afterwards? However I don't run into this for projects that use the ESLint extension.

spence-s commented 3 years ago

@novemberborn - I just cloned ava and installed deps - then ran node_modules/.bin/xo from the command line and get about 120 import/order errors. I can go through each file reported from the command line and see the same report from the extension in vscode. I then look at your configuration - and the very top rule says to error for import/order and it looks like exactly what xo is doing from both the command line and from the extension.

Can you tell me specifically what file is looking incorrect to you?

spence-s commented 3 years ago

Maybe I am confused - maybe you are reporting on the problem that the lint errors are correct, but formatting still leaves another error - and the file doesn't get fully fixed?

If you format 2x it will fix the issues completely. This is a bug with the extension, can you confirm that this is the bug that you are experiencing?

novemberborn commented 3 years ago

npx xo lib/cli.js reports no errors, npx xo --fix lib/cli.js makes no changes. When I open that same file in VSCode the Problems tab reports XO(import/order) errors on these three lines: https://github.com/avajs/ava/blob/a5ac79eb4a69066f166956da3277227bc241b54d/lib/cli.js#L2-L4

I then save (repeatedly) and the imports reorder to be completely alphabetical. Of course at this point npx xo lib/cli.js starts failing:

  lib/cli.js:5:1
  ✖  5:1  There should be at least one empty line between import groups  import/order
  ✖  6:1  node:fs import should occur before import of arrify            import/order
  ✖  7:1  node:path import should occur before import of arrify          import/order
  ✖  8:1  There should be at least one empty line between import groups  import/order
  ✖  8:1  node:process import should occur before import of arrify       import/order

My workspace configuration:

{
    "editor.formatOnSave": true,
    "eslint.enable": false,
    "xo.format.enable": true,
    "xo.enable": true,
    "[javascript]": {
        "editor.defaultFormatter": "samverschueren.linter-xo",
    },
    "editor.codeActionsOnSave": {
        "source.organizeImports": false
    },
    "prettier.enable": false
}
spence-s commented 3 years ago

Thanks so much for the detailed steps. This one was really bugging me as I could NOT reproduce your steps. The extension and the cli were 100% consistent for me.

However, I figured out the problem.

The problem is that eslint-plugin-import has some weird behavior that is not consistent across all versions of node. For node v14, I don't believe it properly supports the "node protocol" import syntax. However, is does properly support it if you are using node v16+.

The reason it is showing up in vscode and not in the cli for you - I would bet because you are using node v16 for cli but vscode is internally using its own bundled version nodejs which is node v14 (which is what I was using for the cli, and why I didn't see the problem initially).

Since eslint-plugin-import does not support the node import protocol for node 14 - it causes weird behavior within vscode only. If you downgrade your system node version to 14, and then run the cli again, you will see the same results that you see in the editor.

for reference see: https://github.com/import-js/eslint-plugin-import/issues/2035

since eslint-plugin-import has questionable support for the node protocol - your best bet is to disable the rule: "unicorn/prefer-node-protocol": "off"

Whenever I do this, everything works as expected.

The eslint extension jumps through more hoops than this extension does to resolve the system versions of node and allows users to give their own path to the node runtime. Because of this bug - I will leave this open and implement the same logic for this extension. However, it may some time until I get to it. For now the best course of action unfortunately is to disable this rule, I will add a note for this behavior on the readme.

TL;DR My guess is that you are using node version 16+ while vscode internally uses its own node v14 which causes different behavior for eslint-plugin-import based on the "node protocol". Disable the node protocol rule like so: "unicorn/prefer-node-protocol": "off" for best consistency until I can get it fixed in the extension.

spence-s commented 3 years ago

Actually - disabling that rule will still require you to rewrite all your imports manually to fix the vscode behavior... I am going to add the runtime option in now. Hold off on doing a rewrite - I will reply here with the correct solution soon so that you will get consisten results.

spence-s commented 3 years ago

@novemberborn

This option is in now.

1) Update linter-xo in vscode to 3.6.

2) Go to command line and run which node (or whatever method you have to get absolute path to node binary you want to use)

3) go to your vscode settings, probably user settings so this will apply to all workspaces, and add "xo.runtime": "/path/to/node

4) Restart VSCode for this to take effect.

5) Open the XO output channel and ensure that the top line says "XO Server Starting in Node v{same version from cli}"

-- I just tested this on ava and it seems to work great for me now. Thanks for your patience with this!

novemberborn commented 3 years ago

Wow, thanks for digging into this @Spence-S! Kinda weird how that lint rule is Node.js version dependent.