-
Notifications
You must be signed in to change notification settings - Fork 136
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 TypeScript project language detection #3604
Conversation
@@ -24,7 +24,10 @@ export async function detectProjectLanguage(context: IActionContext, projectPath | |||
if (await isTypeScriptProject(projectPath)) { | |||
detectedLangs.push(ProjectLanguage.TypeScript); | |||
} else { | |||
detectedLangs.push(ProjectLanguage.JavaScript); | |||
// don't add JavaScript if TypeScript is already detected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if TypeScript is detected (tsconfig found, etc.)
I don't think it's checking for tsconfig right now, but that's an interesting idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I thought we were doing that. Adding a check for a tsconfig would also satisfy these tests since a tsconfig is created upon initializing the test folder.
@@ -24,7 +24,10 @@ export async function detectProjectLanguage(context: IActionContext, projectPath | |||
if (await isTypeScriptProject(projectPath)) { | |||
detectedLangs.push(ProjectLanguage.TypeScript); | |||
} else { | |||
detectedLangs.push(ProjectLanguage.JavaScript); | |||
// don't add JavaScript if TypeScript is already detected | |||
if (!detectedLangs.includes(ProjectLanguage.TypeScript)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isTypeScriptProject
currently checks for typescript
listed as a dev dependency. Isn't that required for TypeScript projects? At the end of the day, it seems fine if we prompt for JS vs TS for weird projects - and I think the unit tests might have weird projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a */index.ts
file, but typescript
is not a dev dependency, then we will detect JS and TS. Whereas before we only detected TS.
Although the test case is weird, if we find a */index.ts
file, I think it's totally safe to assume the project language isn't JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong preference from me 🤷♂️
Changes in #3586 changed TypeScript language detection in that JavaScript would also be detected, which results in prompting the user to choose a language in cases that didn't before #3586 was merged. I found this while fixing the unit tests.
These changes fix the unit tests so that if TypeScript is detected (tsconfig found, etc.), then JavaScript won't also be detected.