sublimelsp / LSP-typescript

TypeScript, JavaScript support for Sublime LSP plugin
MIT License
135 stars 11 forks source link

Sometimes adding (unhelpful) imports on tab-complete in .js files #151

Closed Jimbly closed 10 months ago

Jimbly commented 2 years ago

I installed this and started using it for working in TypeScript files, and it's been working great, and have (mostly) been enjoying the added functionality in .js files as well. However, while tab-completing, it keeps on unhelpfully adding import statements - they end up in the wrong spot in many files, and in the wrong syntax (adding import statements in older modules that must run with node/browserify require() statements), and, even in .ts files, usually in the wrong place (not respecting the eslint-import/order rule defined in the project).

Is there a way to turn this particular feature off, (ideally just in .js files, but probably globally would be fine)?

rchl commented 2 years ago

I think it should be this one: https://github.com/sublimelsp/LSP-typescript/blob/3fc2d53c5aa78cdb3a2eb4e69cb8071fb078929b/LSP-typescript.sublime-settings#L16-L16

As for require vs. import, typescript should be auto-detecting which one to insert. So maybe your code is mixing CommonJS and ES Imports syntax in the same file.

https://user-images.githubusercontent.com/153197/175392256-f9c6f1d5-a833-42a0-8a38-1c7a675e5ab3.mov

Jimbly commented 2 years ago

Thanks for responding!

Hmm, I tried turning that option off, and it seems to mostly just stops suggesting the auto-completes altogether, which isn't desirable, as the auto-complete is handy, the modifying other code out of view less so =).

As for require vs import, we're not mixing those, however we're using Babel and using the export keyword, so perhaps that's throwing it off.

Also narrowed down another case where it's unreliable - if you import a single .js file, it seems to only sometimes add the import, but if referenced using module syntax, it almost always does. Recording 2022-06-23 at 16 52 37 However, even if that's fixed, I'd still like to just turn it off =).

rchl commented 2 years ago

I think it's expected that it stops suggesting un-imported completions with includeCompletionsForModuleExports disabled. I feel that it would be against its philosophy if it would provide completion that would result in invalid code (because of the module not being imported).

If you disagree with that then you could argue on the Typescript repo as it's not something that this server controls.

rchl commented 2 years ago

As far as the behavior being a bit erratic, I believe that is due to editing the code "too fast". It requires the server to provide up to date diagnostics and just ignores the extra changes if completion is triggered before pending diagnostics have been received.

Jimbly commented 2 years ago

I think it's expected that it stops suggesting un-imported completions with includeCompletionsForModuleExports disabled. I feel that it would be against its philosophy if it would provide completion that would result in invalid code (because of the module not being imported).

It's already providing "invalid" code (e.g using "import" in files where it is not supported, placing the imports before exports breaking any circular dependencies, etc) in .js files, though I agree in .ts files it probably makes sense to always do that, as they're much more well-defined than .js files =).

Hmm, these actually the same thing? One is providing completions to the list of auto-complete options, the other is modifying code out of view, which seem pretty functionally unrelated, but I'm not familiar with how Sublime plugins work at this point =).

Alternatively, is there a way to just turn this off in .js files / alter the LSP-typescript settings on a per-file-type basis? Seems to generally work okay in .ts files, just causing headaches in .js files.

rchl commented 2 years ago

It's already providing "invalid" code (e.g using "import" in files where it is not supported, placing the imports before exports breaking any circular dependencies, etc) in .js files

Such issues would have to be looked at individually and checked to see where the fault is. But most likely those are not the kind of issues that Typescript itself sees as invalid, even if those are invalid in your project setup.

You seemed to have said that you are using export keyword in your files so I would only expect that it will add import's since mixing CommonJS and ES Imports is not technically valid (even if Babel might be less strict here).

Hmm, these actually the same thing? One is providing completions to the list of auto-complete options, the other is modifying code out of view, which seem pretty functionally unrelated, but I'm not familiar with how Sublime plugins work at this point =).

As I said, providing completion for a module that is not imported would result in clearly invalid code (showing error diagnostic) so those are quite related IMO. And it's not really something specific to Sublime plugins. It's the functionality of the LSP server https://github.com/typescript-language-server/typescript-language-server

Alternatively, is there a way to just turn this off in .js files / alter the LSP-typescript settings on a per-file-type basis? Seems to generally work okay in .ts files, just causing headaches in .js files.

Those settings can't be set per-syntax unfortunately. Neither ST LSP client nor the underlying LSP server provide that.

Qrokqt commented 1 year ago

I'm also having this issue, we're using a webpack plugin so that paths can start with @ instead of being relative. This is instead inserting incorrect relative paths. I'd like to have the auto completions but don't want it to automatically any imports.

rchl commented 10 months ago

If there is anything that can be improved in those cases then it would be in typescript itself so I'll close this issue.