Skip to content

fix: use workspace version of typescript server in yarn pnp mode #1763

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: use workspace version of typescript server in yarn pnp mode #1763

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions common/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import * as fs from 'fs';
import * as path from 'path';

/**
* Represents a valid node module that has been successfully resolved.
Expand All @@ -17,19 +18,23 @@ export interface NodeModule {
version: Version;
}

export function resolve(packageName: string, location: string, rootPackage?: string): NodeModule|
undefined {
export function resolve(packageName: string, location: string, rootPackage?: string): NodeModule |
undefined {
const isYarnModule = location.startsWith('.yarn/sdks');
location = isYarnModule ? '.yarn/sdks' : location;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix looks reasonable to me- but would there also be a way of just respecting the VSCode typescript.tsdk setting? This is the one that Yarn sets as well and this would be more flexible without hard-coding to Yarn.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why i was a little hesitant to make a PR, it's a bit of a dirty fix. I went for the jugular ^^
Anyway, thanks @atscott for the proper fix !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ld210 PRs serve to get the ball rolling :) Thanks for the detailed information in your issue report. It helped a lot to reproduce things on my end.

rootPackage = rootPackage || packageName;
try {
const packageJsonPath = require.resolve(`${rootPackage}/package.json`, {
paths: [location],
});
const packageJsonPath = isYarnModule ? path.resolve(`${location}/${rootPackage}/package.json`) :
require.resolve(`${rootPackage}/package.json`, {
paths: [location],
});
// Do not use require() to read JSON files since it's a potential security
// vulnerability.
const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
const resolvedPath = require.resolve(packageName, {
paths: [location],
});
const resolvedPath =
isYarnModule ? path.resolve(`${location}/${packageName}`) : require.resolve(packageName, {
paths: [location],
});
return {
name: packageName,
resolvedPath,
Expand Down