fix(lsp): move sloppy import resolution from loader to resolver #23751
Conversation
cli/lsp/documents.rs
Outdated
There was a problem hiding this comment.
Before this PR -> ./foo.js resolves to ./foo.d.ts
After this PR ->./foo.js resolves to ./foo.js
This will cause typescript to resolve the incorrect file I think? Sloppy imports is informed by the provided ResolutionMode that will resolve .d.ts for ResolutionMode::Types and .js for ResolutionMode::Execution. Probably we need to surface that up to a higher level instead of only always doing ResolutionMode::Types (see in resolve_unstable_sloppy_import).
There was a problem hiding this comment.
Hmm my thinking was that this matches the behavior without sloppy imports enabled. Without sloppy imports we don't resolve foo.js to foo.d.ts, since we don't actually do any special resolution for file specifiers. So it seems inconsistent to change that behavior for an import that isn't actually sloppy
There was a problem hiding this comment.
Yeah it's different than Deno's default resolution. For sloppy imports it does TypeScript's Node resolution behaviour where it prefers .d.ts files when resolving the types, but .js files when executing the code.
There was a problem hiding this comment.
This actually might be a bug/design issue elsewhere as well. This is a fully resolved specifier so it's strange sloppy import resolution is happening here (in resolve_specifier). Probably it should only happen when resolving import specifiers (ex. code with (specifier: &str, base: &Url) params)
There was a problem hiding this comment.
I did a commit that moves the sloppy import resolution from the loader to the resolver. This should fix the issue if you try it out now (and resolve to the .d.ts file).
This also seems to fix #23197 for me.
dsherret
changed the title
cli/lsp/resolver.rs
Outdated
| &self, | ||
| // this really only needs a HashSet<ModuleSpecifier>, but it's provided | ||
| // the entire HashMap to avoid cloning all the time | ||
| open_docs: &'a HashMap<ModuleSpecifier, Arc<Document>>, |
There was a problem hiding this comment.
This is only necessary for sloppy import resolution. It's slightly unfortunate to have to have to pass it around, but I'm not sure a better way because we're mutating the Documents while calling this sometimes and sloppy import resolution needs to know about files that may only exist in memory.
| @@ -12106,7 +12049,7 @@ fn lsp_deno_future_env_byonm() { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn lsp_sloppy_imports_warn() { | |||
There was a problem hiding this comment.
I removed the warning. It's not worth maintaining because it's a lot of work to add when this is done from the resolver. We can add this back as a lint rule later (see #22844 (comment))
| Ok( | ||
| SloppyImportsResolver::resolve_with_stat_sync( | ||
| &specifier, | ||
| mode, |
There was a problem hiding this comment.
See here how this now properly handles the mode for resolution.
cli/lsp/resolver.rs
Outdated
| &'a self, | ||
| // this really only needs a HashSet<ModuleSpecifier>, but it's provided | ||
| // the entire HashMap to avoid cloning all the time | ||
| open_docs: &'b HashMap<ModuleSpecifier, Arc<Document>>, |
There was a problem hiding this comment.
Was thinking about this more this morning and realized that we should just not bother passing the open documents here. In places like vscode, the file will always exist on the file system if it's open (unlike our test server), so we can just consult that for simplicity.
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
One thing we're going to have to do (and this is an existing bug) is recreate the resolved resolutions when a file is deleted or created, but that's a problem for a different day.
Moves sloppy import resolution from the loader to the resolver. Also adds some test helper functions to make the lsp tests less verbose --------- Co-authored-by: David Sherret <[email protected]>
Fixes #23745
Fixes #23197
Moves sloppy import resolution from the loader to the resolver.
Also adds some test helper functions to make the lsp tests less verbose