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

LSP-eslint exits unexpectedly in ST 4098 due to non-conforming stdout input #1612

Open
tamamadesu opened this issue Mar 10, 2021 · 17 comments · May be fixed by #2015
Open

LSP-eslint exits unexpectedly in ST 4098 due to non-conforming stdout input #1612

tamamadesu opened this issue Mar 10, 2021 · 17 comments · May be fixed by #2015

Comments

@tamamadesu
Copy link

in .TSX file , Here is the startup console:
image

LSP-eslint.sublime-settings:
{ "enabled": true, "settings": { "validate": [ "javascript", "javascriptreact", "vue", "typescript", "typescriptreact" ], "options": { "parserOptions": { "project": [ "./tsconfig.json" ], } }, } }

Need some help, Thx !

@rchl
Copy link
Member

rchl commented Mar 10, 2021

If it shows up in the lower-left corner of ST window then it means it's working. It's just a matter of configuring eslint through project-specific eslint configuration to make it report errors.

Stuff like parserOptions would make more sense in the project-specific eslint settings IMO (.eslintrc.js).

@tamamadesu
Copy link
Author

Thx reply

fisrt the same eslint setting in vscode was worked,
in my test case,if set parser: "@typescript-eslint/parser" in .eslintrc,when open .tsx file,the lower-left corner ' LSP-eslint' just show a little second, then disappeared and no any console, but if delete the parse,it works

@rchl
Copy link
Member

rchl commented Mar 10, 2021

are you able to provide a github repository that reproduces the issue?

@tamamadesu
Copy link
Author

are you able to provide a github repository that reproduces the issue?

ok

@tamamadesu
Copy link
Author

here is a test demo: https://github.com/tamamadesu/eslint-test

@rchl
Copy link
Member

rchl commented Mar 11, 2021

LSP-eslint doesn't seem to report any error and just crashes silently.

But if you try to run eslint manually then you'll see what the error is:

npx eslint --ext .tsx .
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.2.1 <3.4.0

YOUR TYPESCRIPT VERSION: 3.9.9

Please only submit bug reports when using the officially supported version.

=============

/temp/eslint-test/index.tsx
  0:0  error  Parsing error: ImportDeclaration should appear when the mode is ES6 and in the module context

✖ 1 problem (1 error, 0 warnings)

You are using very old versions of both eslint, typescript, and typescript-eslint plugins. Try to upgrade them all to the latest versions.

@rchl
Copy link
Member

rchl commented Mar 11, 2021

The fact that it works in VSCode is a bit of a mystery for me right now. I'll have a look at it later.

@rchl
Copy link
Member

rchl commented Mar 11, 2021

It appears that this warning (the one starting with ============= line) is printed to stdout and our implementation is strict in the sense that it requires properly formatted RPC messages. So it just stops on that line and closes the connection...

@rchl rchl transferred this issue from sublimelsp/LSP-eslint Mar 11, 2021
@rchl rchl changed the title It doesn't seem work in ST 4098 LSP-eslint exits unexpectedly in ST 4098 due to non-conforming stdout input Mar 11, 2021
@rchl
Copy link
Member

rchl commented Mar 11, 2021

Moved this to the LSP repo.

At the minimum, we should print the exception to make cases like this easier to diagnose in the future.

Possibly we can gracefully handle such cases and just print those non-recognized messages to the LSP log panel.

@tamamadesu
Copy link
Author

LSP-eslint doesn't seem to report any error and just crashes silently.

But if you try to run eslint manually then you'll see what the error is:

npx eslint --ext .tsx .
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.2.1 <3.4.0

YOUR TYPESCRIPT VERSION: 3.9.9

Please only submit bug reports when using the officially supported version.

=============

/temp/eslint-test/index.tsx
  0:0  error  Parsing error: ImportDeclaration should appear when the mode is ES6 and in the module context

✖ 1 problem (1 error, 0 warnings)

You are using very old versions of both eslint, typescript, and typescript-eslint plugins. Try to upgrade them all to the latest versions.

Ok, Thx!!!

@rchl
Copy link
Member

rchl commented Mar 11, 2021

Actually, VSCode uses IPC transport and not STDIO so it's not affected by this issue.

I'm not sure we can or should even try to fix it as the fix would be against the spec.

We can probably only just make sure that the error is surfaced.

@alecmev
Copy link
Contributor

alecmev commented Aug 7, 2022

Continuing from #2013.

vscode handles it fine. Looks like it ignores anything that can't be parsed as LSP messages.

As noted in the comment above, indeed, VSCode doesn't have this issue because it uses IPC. The client sets the transport kind here, and it executes the server with --node-ipc here, and sets up the reader here, implemented here. There you can see that it uses serverProcess.on('message', ...), no stdout involved. Wish I saw this issue before the rabbit hole 😅

such content should not be included in stdout so in theory that is something that should be fixed in https://github.com/microsoft/vscode-eslint or maybe even eslint itself

This is possible (ESLint could invoke configuration files and plugins in a separate process and pipe the stdout only if asked to, and vscode-eslint could communicate with eslint/lib/api.js through a separate process too), but I'm not sure if such changes will ever be accepted, since both of them work fine as-is.

I'm thinking of adding IPC support instead, it actually looks pretty doable!

@rchl
Copy link
Member

rchl commented Aug 7, 2022

Wish I saw this issue before the rabbit hole 😅

Wish I would remember that I've investigated it before. :)

@rchl
Copy link
Member

rchl commented Aug 7, 2022

I'm thinking of adding IPC support instead, it actually looks pretty doable!

Well, it would need to be done in python though and that's probably "a bit" more work as you can't rely on anything built-in into node.

@alecmev
Copy link
Contributor

alecmev commented Aug 7, 2022

It's actually not bad, here's a PoC:

# foo.py

import json
import multiprocessing
import os
import subprocess

parent_conn, child_conn = multiprocessing.Pipe()
env = os.environ.copy()
env['NODE_CHANNEL_FD'] = str(child_conn.fileno())

subprocess.Popen(
    ['node', 'foo.js'],
    pass_fds=(child_conn.fileno(),),
    env=env,
)

parent_conn._write(parent_conn.fileno(), json.dumps('Hello from Python!').encode("ascii") + b'\n')
print('Python received: ' + json.loads(parent_conn._read(parent_conn.fileno(), 1000)))
// foo.js

process.on('message', (m) => console.log(`JavaScript received: ${m}`));
process.send('Hello from JavaScript!');
# python foo.py
Python received: Hello from JavaScript!
JavaScript received: Hello from Python!

NODE_CHANNEL_FD is from here. The IPC serialization is super simple, just newline-trailing JSON strings. Using multiprocessing.Pipe because it's a duplex and multi-platform. parent_conn._read and parent_conn._write because they use custom code on Windows.

Should I proceed with this?

@rchl
Copy link
Member

rchl commented Aug 8, 2022

Any help here is appreciated so yeah :)

If that's all it takes then it should be pretty easy to integrate it with the LSP code base and add it as a new client configuration option.

Feel free to ask if you have any questions. You can also create a draft PR to make collaboration easier.

alecmev added a commit to alecmev/LSP that referenced this issue Aug 9, 2022
alecmev added a commit to alecmev/LSP that referenced this issue Aug 9, 2022
@alecmev alecmev linked a pull request Aug 9, 2022 that will close this issue
@alecmev
Copy link
Contributor

alecmev commented Aug 9, 2022

It works! #2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants