Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dependencies should be project-specific (wrong dependency with useWorkspaceDependencies) #3137

Closed
4 tasks done
phil294 opened this issue Sep 4, 2021 · 6 comments
Closed
4 tasks done
Assignees
Milestone

Comments

@phil294
Copy link
Contributor

phil294 commented Sep 4, 2021

  • I have searched through existing issues
  • I have read through docs
  • I have read FAQ
  • I have tried restarting VS Code or running Vetur: Restart VLS

Info

  • Platform: Linux (Does not matter)
  • Vetur version: v0.34.1
  • VS Code version: 1.59.0

Problem

When you set "vetur.useWorkspaceDependencies": true, Vetur will scan your entire workspace for dependencies. Dependencies are not specific to project, as projectService imports dependencyService globally.

This is problematic for example when you have an backend and an app folder in your workspace; Vue resides in app. Vetur will scan both folders and you might end up with TS from backend, even though you may have specified a specific TS version in your WEB package.json. If both TS versions have equal dependency path depth, their precedence even relies on their alphabetic ordering, if I figured the code correctly, which is of course random and hard to track down.

Reproducible Case

Here is a repro demo repo: https://github.com/phil294/vetur-dependency-error-demo
If you go into /app/sfc.vue, you will find that line 4 (a ||= b) will show an error. It is TS 4.x syntax. app has TS 4.x, the unrelated backend uses 3.x.

According to extension output channel, these TS versions are being loaded:

[INFO ] Loaded typescript@4.4.2 from /tmp/vetur-dep-problem/app/node_modules/typescript.
[INFO ] Loaded typescript@3.9.10 from /tmp/vetur-dep-problem/backend/node_modules/typescript.

backend comes second due to natural ordering, and only the last one is used by Vetur:

return deps[deps.length - 1];

@yoyo930021 yoyo930021 self-assigned this Oct 4, 2021
@yoyo930021 yoyo930021 added this to the v0.35.0 milestone Oct 4, 2021
@phil294
Copy link
Contributor Author

phil294 commented Oct 6, 2021

@yoyo930021 thanks for looking into this, but unfortunately, it's not fixed. There's still alphabetical ordering going on, but now the other way round.

I assume you used my demo repo as a test case. If you rename the folder backend to api (so it now comes before app), its TS version will be used once again (because possiblePaths is empty), when it should actually be ignored regardless of its name.

const possiblePaths: string[] = [];
let tempPath = path.dirname(filePath);
while (
rootPathForConfig === tempPath ||
getPathDepth(rootPathForConfig, path.sep) > getPathDepth(tempPath, path.sep)
) {
possiblePaths.push(path.resolve(tempPath, `node_modules/${lib}`));
tempPath = path.resolve(tempPath, '../');
}
const result = deps.find(dep => possiblePaths.includes(dep.dir));
return result ?? deps[0];

Probably the problem is here. I don't really understand the point of the loop :-/ But I think it's fixed by changing > to <

@yoyo930021 yoyo930021 reopened this Oct 7, 2021
@yoyo930021
Copy link
Member

I will look your case.

@yoyo930021
Copy link
Member

yoyo930021 commented Oct 7, 2021

@yoyo930021 thanks for looking into this, but unfortunately, it's not fixed. There's still alphabetical ordering going on, but now the other way round.

I assume you used my demo repo as a test case. If you rename the folder backend to api (so it now comes before app), its TS version will be used once again (because possiblePaths is empty), when it should actually be ignored regardless of its name.

const possiblePaths: string[] = [];
let tempPath = path.dirname(filePath);
while (
rootPathForConfig === tempPath ||
getPathDepth(rootPathForConfig, path.sep) > getPathDepth(tempPath, path.sep)
) {
possiblePaths.push(path.resolve(tempPath, `node_modules/${lib}`));
tempPath = path.resolve(tempPath, '../');
}
const result = deps.find(dep => possiblePaths.includes(dep.dir));
return result ?? deps[0];

Probably the problem is here. I don't really understand the point of the loop :-/ But I think it's fixed by changing > to <

This commit fixes it.
When old code:
截圖 2021-10-07 下午4 04 10
When new commit:
截圖 2021-10-07 下午4 05 19

This problem is solved when the issue is closed, but it isn't released yet.
Please wait for the new version to be released.

@phil294
Copy link
Contributor Author

phil294 commented Oct 7, 2021

This problem is solved when the issue is closed, but it isn't released yet. Please wait for the new version to be released.

I'm afraid there has been a misunderstanding. I know it's not released yet, I tried your commit from the master branch by building Vetur on my machine. Yes, it fixes the error in the test repo. But when you rename the folder backend to api, there is an error again in app/sfc.vue.

I can create another repo if you want?

@yoyo930021
Copy link
Member

yoyo930021 commented Oct 7, 2021

This problem is solved when the issue is closed, but it isn't released yet. Please wait for the new version to be released.

I'm afraid there has been a misunderstanding. I know it's not released yet, I tried your commit from the master branch by building Vetur on my machine. Yes, it fixes the error in the test repo. But when you rename the folder backend to api, there is an error again in app/sfc.vue.

I can create another repo if you want?

Welcome, I will take a look when free.
In the new version, vetur will use the project package.json file path to find typescript module.

phil294 added a commit to phil294/coffeesense that referenced this issue Oct 13, 2021
…accidental TS versions from sibling folders)

Fixed in upstream, adopted vuejs/vetur#3137
yoyo930021 added a commit that referenced this issue Oct 16, 2021
@yoyo930021
Copy link
Member

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants