Types for built-in elements getting overriden

[joelmukuthu]

Describe the bug
If one installs some @types/* npm module that somehow overrides React types then type-checking gets messed up. I can imagine why this happens, but it would be nice if it could be fixed somehow. In my case, I'm using storybook (with Svelte) but it installs a bunch of @types/* modules which somehow mess up the types that the language server users. I haven't identified which of the types modules is the culprit. But ideally the language server types should trump other types.

To Reproduce
Steps to reproduce the behavior:

1. Install the @storybook/addon-knobs npm module
2. Trigger TS types to be loaded somehow, e.g. by importing the module
3. Simple HTML now triggers type errors

Example code that triggers the error:

<script lang="typescript">
  import * as foo from '@storybook/addon-knobs';
</script>

<div>
  <p></p>
</div>

Expected behavior
That the HTML would not trigger an error.

Screenshots

System (please complete the following information):

• OS: Linux
• IDE: VSCode
• Plugin/Package: "Svelte Beta", svelte-language-server, svelte-check

Additional context
svelte-check also triggers the same errors that show up in the IDE.

[dummdidumm]

The module loads global type definitions, some of the React type definitions. There are other people having this problem, seems there is no good solution to this yet, but workarounds exist
microsoft/TypeScript#17042
microsoft/TypeScript#18588

There is a possible workaround, maybe you can try that yourself. We might also add that workaround to the repo if it's doable.

[joelmukuthu]

Thanks for the heads up @dummdidumm. That worked! For reference:

tsconfig.json:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "react": ["sink.d.ts"]
    }
  }
}

sink.d.ts:

declare module 'react';

Is that something that could be added to the language server itself or would it mess things up for a user that actually wanted to use React's types on their project?

[dummdidumm]

I don't think so. Even if he does, the language server only runs on svelte files, so if he is in a tsx/jsx file, everything should work. I'll add something.

[dummdidumm]

You can test this on nightly build.

[joelmukuthu]

Is it already deployed to the nightly build? It doesn't seem to work unfortunately. However, I think baseUrl is required if paths is also configured in tsconfig.json https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping. I can try to add the baseUrl to my local environment to verify if that'll make it work.

[joelmukuthu]

Oh, never mind. Seems that changes haven't been deployed to the nightly build yet. I'll test it again tomorrow.

[dummdidumm]

Thanks for pointing that out, there was an error in the deploy pipeline (order was messed up, so the extension was built with the npm packages from the day before).

[joelmukuthu]

Sorry for the slow response @dummdidumm. I tested the fix and unfortunately it doesn't work, but I played around and found the necessary fixes:

1. Here, it should be configJson.compilerOptions.paths instead of configJson.paths. Small bug :)
   

   language-tools/packages/language-server/src/plugins/typescript/service.ts

   Lines 189 to 190 in 9b936fc

   	configJson.paths = configJson.paths || {};
   	configJson.paths.react = [

2. It doesn't work without a configJson.compilerOptions.baseUrl option. That's a little harder to fix. First, we need to check if one is set already and if not, the happy path is of course configJson.compilerOptions.baseUrl = '.'. However, if it is set, we have to resolve our path relative to whatever the user set as their base url. That makes things a bit more complicated but I'm sure there's a way to engineer it. I don't have time to look into atm but I wanted to bring it to your attention.

[dummdidumm]

I'm not sure about needing baseUrl, because the types that are getting set have an absolute path. I think the ts service honors that.

[dummdidumm]

Thanks for pointing out the compilerOptions mistake 😃 Fixing that solved the typing issue.

Note about svelte-check: If you still experience this error tomorrow, but not in the IDE anymore, try uninstalling and reinstalling svelte-check, the transitive svelte-language-server dependency might not get updated to the latest version.

[joelmukuthu]

Thanks for the follow up @dummdidumm! I tested it once again and can confirm for certain that the fix doesn't work if compilerOptions.baseUrl is not set 😀 However, the good news is that if the user has set some baseUrl, it doesn't affect the resolution of the react override since that's an absolute path. I was wrong in thinking that we'd need some hackery to get it to work.

If you tested the current solution and got it to work, check that there's no compilerOptions.baseUrl configured in your test environment's tsconfig.json ;)

[dummdidumm]

Docs are added. I will leave this open for now so it's easier to discover in case someone gets the error but does not read the troubleshooting section.

[dummdidumm]

Great news, this was fixed as a side effect of #351, after the next production release, the workaround with sinks is no longer necessary.

[joelmukuthu]

Wonderful! Thanks for the heads-up! 🎉