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

Allow VS Code Plugin to use workspace Typescript version #1045

Open
danieldiekmeier opened this issue Jun 3, 2021 · 8 comments
Open

Allow VS Code Plugin to use workspace Typescript version #1045

danieldiekmeier opened this issue Jun 3, 2021 · 8 comments
Labels
feature request New feature or request

Comments

@danieldiekmeier
Copy link
Contributor

Describe the bug
The current version of Zod has a compatibility problem with Typescript 4.3. (colinhacks/zod#473)

This is not a big problem, I can just keep using Typescript 4.2 until the problem is fixed by one of the projects. In .ts files, this works fine.

But in Svelte files, I still get the Excessive stack depth error from Typescript 4.3.

I noticed that

defines the Typescript version as '*', and looking on my hard drive at the extension files ~/.vscode/extensions/svelte.svelte-vscode-105.1.0/node_modules/typescript/package.json, I can see that it indeed installed Typescript 4.3.2

Is it possible that the Extension uses this Typescript version, and thus shows me the error?

To Reproduce

I made a very small reproduction: https://github.com/danieldiekmeier/svelte-plugin-uses-wrong-typescript-version-reproduction

For me, I only need to open the project folder in VS Code and I get the red underline like in the screenshot above.

Expected behavior

I expect the extension to use the Typescript version I configured in VS Code (especially if I set it to use the version from my node_modules).

@danieldiekmeier danieldiekmeier added the bug Something isn't working label Jun 3, 2021
@danieldiekmeier danieldiekmeier changed the title I think the VS Code Plugin uses its own Typescript version I think the VS Code Plugin doesn't use the correct Typescript version Jun 3, 2021
@dummdidumm dummdidumm changed the title I think the VS Code Plugin doesn't use the correct Typescript version Allow VS Code Plugin to use workspace Typescript version Jun 3, 2021
@dummdidumm dummdidumm added feature request New feature or request and removed bug Something isn't working labels Jun 3, 2021
@dummdidumm
Copy link
Member

dummdidumm commented Jun 3, 2021

Right now the plugin is hardwired to the version it comes packaged with. Adding support for this means rewriting all places where TypeScript is imported to use a injected TS version instead - those are a lot of places.
As a quick fix, you can try going back to previous versions of the extension where a TS version lower than 4.3 is packaged.

@danieldiekmeier
Copy link
Contributor Author

Okay, I understand! I hope that it will become unnecessary very soon, when either Zod or Typescript can figure out their differences.

I think I can't really go back to a previous version, because of the other error I recently bumped into.

But I figured out that if I do

cd ~/.vscode/extensions/svelte.svelte-vscode-105.1.0
npm i -D [email protected]
npm i # before i did this, lodash was missing?

the extension somehow still works and now behaves correctly. This is probably not a great idea and if it breaks, it's my own fault, but at least I can now continue to work for the time being :D

@pheuter
Copy link

pheuter commented Nov 18, 2021

Apologies if the bump is inappropriate, but I too would find value in being able to use the workspace TypeScript version with the Svelte extension. TypeScript 4.5 was just released and being able to have interop across .ts and .svelte files would be nice.

@remyrylan
Copy link

@dummdidumm is there any technical limitation behind the extension depending on its own version of TypeScript instead of being able to attach to the version provided by VS Code? If not, I'm interested in trying to submit a patch.

@dummdidumm
Copy link
Member

dummdidumm commented Dec 10, 2021

There's no technical limitation, but as I said above, adding support for this means rewriting all places where TypeScript is imported to use a injected (via function or constructor parameter) TS version instead - those are a lot of places.

I just realized another possibility to use the TypeScript version with resorting to hacks:

  1. Install svelte-language-server as a dev dependency in the project where you want to use the workspace version
  2. Use the language server path setting and enter the path to the language server executable, for example ./node_modules/svelte-language-server/bin/server.js

I feel like this is an acceptable solution for people to use the workspace version of TypeScript, given it's a rather niche use case - and it's actually the same way that VS Code supports setting the TypeScript workspace version. What do others think about this?

@remyrylan
Copy link

@dummdidumm That's a very reasonable workaround that likely resolves things for most users.

I'll be totally transparent in what I'm trying to address related to this issue -- I'm in the (currently) super niche group of people that are using Deno + Svelte. I'm building a starter kit/framework/whatever combining Svelte and Deno and I want to facilitate the best developer experience I can for adopters...

So Deno requires that full sources are specified in module imports/exports. TypeScript forbids using .ts extensions, so the Deno VS Code extension attaches a TS LSP plugin to whichever version of TypeScript is provided by VS Code (built-in version or workspace version). The Deno LSP plugin intercepts TS errors related to module resolution so that you can use the full relative path with a .ts extension.

But what's happening is that the TypeScript LSP launched by the Svelte extension does not use any of the TS plugins supplied by other extensions... so within a Svelte file, the TS LSP knows nothing about Deno, and forbids the use of full filenames with .ts since the Deno TS plugin is not attached to suppress the errors...

It makes for a confusing developer experience for Deno users and people leveraging the experimental custom loaders for Node.js (custom loaders can support compiling TS on the fly, and you can configure your loader to expect full source pathnames with the extension intact)... it's confusing to explain "in a .ts file, use the full extension, but in a Svelte file, make sure you avoid it!"

Of course this could all be solved if the TS team would just allow including .ts in the import/export source path, but that's been requested for years with tons of support and they won't budge on it so far.

The other way this could be fixed would be if preprocessors configured with svelte.config.js were processed before sending the TS code to be type-checked, I threw together a quick Svelte preprocessor that chops off .ts extensions for imports/exports, but the preprocessor runs AFTER the code is sent to TS.

I'm trying to figure out the best approach here -- another idea is to add in the same sort of logic that the Deno TS plugin has and just proxy the TS module resolution function... it's tiny (see below) would you be willing to accept a PR that added the option for the TS plugin to allow for .ts extensions in source paths?

Source for TS plugin to allow for .ts extensions:

const origResolveModuleNames = info.languageServiceHost.resolveModuleNames;

info.languageServiceHost.resolveModuleNames = (
  moduleNames,
  containingFile,
  reusedNames,
  redirectedReference,
) => {
  moduleNames = moduleNames.map((moduleName) => {
    // For any modules ending with `.ts` we will strip that off
    if (moduleName.endsWith(".ts")) {
      const newName = moduleName.slice(0, -3);
      return newName;
    }
    return moduleName;
  });

  // We use the remapped module names to call the original method
  return origResolveModuleNames.call(
    info.languageServiceHost,
    moduleNames,
    containingFile,
    reusedNames,
    redirectedReference,
  );
};

@dummdidumm
Copy link
Member

Thanks for explaining your use case. To me this sounds more like #905 . Passing the TS workspace version likely won't change anything as TS plugins are a editor-only thing, and the svelte-language-server will instanciate a TS language service on its own which has no knowledge of plugins - it's an entirely different instance.
The other approach of using a preprocessor is another limitation which is tracked in #339 . The problem is that TypeScript demands all-synchronous code paths but preprocessors are/can be asynchronous.
Adding the code you mention into the language server hardcoded is the last straw to me, but I would maybe make an exception in this case because, if I understood you correctly, that's what Deno demands and there's a currently otherwise unresolvable conflict.

@remyrylan
Copy link

remyrylan commented Dec 10, 2021

@dummdidumm, you're right it does sound more like #905. I'm sorry for hijacking this issue.

And you do understand correctly. Right now, if you're building a project using Deno + Svelte, and are using the Svelte VS Code extension, you will get TS errors (TS error 2691) for imports that include a ".ts" extension in the import source path.

So Deno + Svelte users are forced to do the following:

  1. Omit the use of .ts extensions in the import sources within Svelte files, which is opposite of Deno, which requires full paths.
  2. Wire up some custom logic or a Svelte preprocessor to add in the .ts extension to import sources that don't have a file extension.... this is risky to do from the perspective of a library because the lib would have to actually try to resolve the modules every single time in order to figure out if it's truly a TS file or not. You couldn't just go off of whether or not there was a period in the filename, someone could be importing import * as foo from "./src/foo.bar"; when the actual filename is ./src/foo.bar.ts.

Just a small note here about Svelte imports such as svelte/internal: Deno can easily handle those via the use of an import_map.json file for dependencies.

So the most future-proof solution is a specific opt-in configuration for people that use Deno or a custom Node.js loader that wish to suppress the TS errors. The code I provided above not only allows a user to specify an import source with a .ts extension intact, it also automatically patches up the autocomplete for an import/export source in VS Code to reference the full file name with the .ts extension.

Like I said above, when using the Deno for VS Code, .ts extensions are allowed with the TS error suppressed but only within .ts and .tsx files.

I know it seems like an edge-case, but people REALLY want to be able to use TS with full paths, and the TS team is not budging on it. It's silly, because people are already doing it, and the TS team won't implement because they want to avoid confusion about not changing import/export source paths at compile time... in the case of Deno and Node.js custom loaders, the TS compiler is never even touched. Many people use SWC in Deno or Babel in Node.js, they are treating TS as only a type checker, not a compiler.

Take a look at these TS issues for how popular of a request this is, it's not exclusive to Deno:
microsoft/TypeScript#27481
microsoft/TypeScript#37582

If you're open to a PR to add a configuration option, I can submit one. I would want to name the option something generic, not specific to Deno, since it is applicable to people using custom Node.js loaders as well. Something like:

Enable literal TypeScript module resolution
"When enabled, this option allows you to include the .ts file extension in your import and export sources within a Svelte component. This is useful for custom Node.js loaders and other JS runtimes which require complete source paths with file extensions."


** EDIT (hours later) **
I've been digging through the Svelte VS Code Extension, I've figured out the .ts extension patching must happen within the Svelte language server, specifically at the source file svelte-language-server/dist/src/plugins/typescript/module-loader.ts.

If I patch, rebuild, and then reload VS Code, everything works. What I'm not quite sure of yet is if any options from the VS Code extension are passed to the language server at all in order to make the functionality opt-in.

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

No branches or pull requests

4 participants