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

(fix) add compilerOptions.baseUrl if compilerOptions.paths is set #261

Closed
wants to merge 2 commits into from

Conversation

joelmukuthu
Copy link

This ensures that the compilerOptions.paths setting is "accepted" by the typescript compiler. Even though the path to the react override is an absolute path, it seems that if no compilerOptions.baseUrl is set, the compiler just ignores compilerOptions.paths altogether.

Follow up to #260 and #234. Fixes #228.

@dummdidumm
Copy link
Member

dummdidumm commented Jul 2, 2020

I tested it without the baseUrl in the other PR and the errors went away for me. I reproduced it like you specified in the issue. For you the error still occured? Edit: Nevermind, as you said in the issue, I need to check my tsconfig.json, it indeed had baseUrl set 😄 Edit 2: I'm not sure if we can add this to the language server then. I did not try it out yet, but what if the user did not specify a baseUrl? Then the IDE might suggest imports like base/..., which the build process will not be able to resolve because he does not know about baseUrl.

@joelmukuthu
Copy link
Author

joelmukuthu commented Jul 2, 2020

You're absolute right! If we set a baseUrl then it does mess with the import suggestions. I think it's the reason the tests are failing. So if there's no baseUrl we should probably set it to the URL of the user's current workspace? From your knowledge of the language server, is that possible? It's what I was trying to do by setting it to . but maybe that was a bit naive 😁 I can look into it this weekend if you agree that that's the right thing to do but you're also very welcome to take over :)

@dummdidumm
Copy link
Member

I just did a small test and confirmed that we cannot set baseUrl ourselves. For example rollup then does not know what this is resolved to, guesses a global variable which is wrong.

So basically we cannot fix this without breaking everyone else 😞 I'm really sorry but I think we cannot add this to svelte-language-server then. The only thing we can do is add a troubleshooting section and add the solution there.

@dummdidumm dummdidumm closed this Jul 3, 2020
@joelmukuthu joelmukuthu deleted the patch-1 branch July 3, 2020 08:04
@joelmukuthu
Copy link
Author

joelmukuthu commented Jul 3, 2020

Thanks a lot for your diligence on this @dummdidumm! It all makes a ton of sense. Since we already have some guides on the svelte-vscode package README I think that would be a good place to add it. I'll send in a PR :)

EDIT: I'll also take out the code you added in #230 if you haven't yet.

@dummdidumm
Copy link
Member

I already created a PR which adds the docs and removes the code #268 - please let me know if the docs are understandable 😃

@joelmukuthu
Copy link
Author

I just saw it 😀 I should really get in the habit of reading all my emails at once. I'll comment there :)

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

Successfully merging this pull request may close these issues.

Types for built-in elements getting overriden
2 participants