Merge pull request #169 from Typeform/WEB-7151/typescript-check-allow…

…-typed-JS

fix(WEB-7151): Typescript check: Allow typed JS

README.md

@@ -67,8 +67,8 @@ documentation](https://github.com/actions/toolkit/blob/master/README.md#packages
 for the various packages.
 
 We have a wrapper for `@actions/github` in
-`.src/infrasctructure/github` that's meant to hold handy helper
-methods for accesing data from GitHub that the action might care about
+`.src/infrastructure/github` that's meant to hold handy helper
+methods for accessing data from GitHub that the action might care about
 (e.g. info about the PR that triggered the action). They also provide
 an extra abstraction layer that is easier to mock in your tests than
 pure Octokit. If you need more info from GitHub in your check,

src/checks/requiredTypeScript.test.ts

@@ -44,6 +44,10 @@ describe('Required TypeScript check', () => {
       mockGithub.getPullRequest.mockResolvedValue(
         pullRequestResponse as PullsGetResponse
       )
+
+      mockReadFile.mockReturnValue(
+        'some js/ts content\nsome more js/ts content'
+      )
     })
 
     it('returns true for PR without JS changes and good tsconfig', async () => {
@@ -80,6 +84,31 @@ describe('Required TypeScript check', () => {
       await expect(requiredTypeScript.run()).resolves.toBeTruthy()
     })
 
+    it('returns true PR with JS+TS changes and good tsconfig', async () => {
+      pullRequestResponse.user.login = 'regular-user'
+      mockGithub.getPullRequestFiles.mockResolvedValue([
+        { filename: 'filename.js', additions: 100, deletions: 20 },
+      ] as PullRequestFiles)
+
+      mockGlob.mockResolvedValue(['tsconfig.json'])
+      mockReadFile.mockImplementation((filename: string) => {
+        switch (filename) {
+          case 'tsconfig.json':
+            return JSON.stringify({
+              compilerOptions: {
+                allowUnreachableCode: false,
+                noImplicitAny: true,
+              },
+            })
+
+          default:
+            return '// @ts-check\nsome js/ts content\nsome more js/ts content'
+        }
+      })
+
+      await expect(requiredTypeScript.run()).resolves.toBeTruthy()
+    })
+
     it('throws error for PR with JS changes but good tsconfig', async () => {
       pullRequestResponse.user.login = 'regular-user'
       mockGithub.getPullRequestFiles.mockResolvedValue([
@@ -366,6 +395,24 @@ describe('isForbiddenJSFile', () => {
     expect(isForbiddenJSFile(filename)).toBeFalsy()
   })
 
+  it.each(['filename.js', 'component.jsx'])(
+    'it returns false with JS/JSX file with TS checks enabled [%s]',
+    (filename) => {
+      expect(
+        isForbiddenJSFile(filename, '// @ts-check\nsome-js-content')
+      ).toBeFalsy()
+    }
+  )
+
+  it.each(['filename.js', 'component.jsx'])(
+    'it returns false with JS/JSX file with TS checks enabled and whitespace around comment [%s]',
+    (filename) => {
+      expect(
+        isForbiddenJSFile(filename, '   //@ts-check\nsome-js-content')
+      ).toBeFalsy()
+    }
+  )
+
   it.each([
     'config.toml',
     'manifest.yaml',
@@ -402,6 +449,32 @@ describe('measureTsAdoption', () => {
       measureTsAdoption().then((x) => formatAdoptionPercentage(x))
     ).resolves.toBe('66.7%')
   })
+
+  it('correctly calculates and formats adoption % with some typed JS files', async () => {
+    mockGlob.mockImplementation(async ({ patterns }) => {
+      if (patterns[0].includes('js')) {
+        return ['file.js', 'file-2.js']
+      } else if (patterns[0].includes('ts')) {
+        return ['file.ts']
+      }
+      return []
+    })
+    mockReadFile.mockImplementation((path) => {
+      switch (path) {
+        case 'file.js':
+          return 'this\nis a\njs\nfile'
+        case 'file-2.js':
+          return '// @ts-check\nthis\nis a\njs\nfile'
+        case 'file.ts':
+          return 'this\nis a\nlonger\nts\nfile\nso it has\nmore\nadoption'
+      }
+      return ''
+    })
+
+    await expect(
+      measureTsAdoption().then((x) => formatAdoptionPercentage(x))
+    ).resolves.toBe('76.5%')
+  })
 })
 
 describe('missingTsConfigSettings', () => {

src/checks/requiredTypeScript.ts

@@ -1,14 +1,15 @@
 import * as core from '@actions/core'
 import * as CommentedJSON from 'comment-json'
-
 import { WebhookEventMap } from '@octokit/webhooks-types'
+import ignore, { Ignore as IgnoredFileFilter } from 'ignore'
 
 import { github } from '../infrastructure/github'
 import * as fs from '../infrastructure/fs'
 import { isBot } from '../conditions/triggeredByBot'
 
 import Check from './check'
-import ignore, { Ignore as IgnoredFileFilter } from 'ignore'
+
+const JS_TS_CHECK_COMMENT_REGEX = /^\s*\/\/\s*@ts-check/gm
 
 type TsConfig = {
   compilerOptions?: {
@@ -39,6 +40,14 @@ const requiredTypeScript: Check = {
 }
 export default requiredTypeScript
 
+const getFileContent = (filename: string) => {
+  try {
+    return fs.readFile(filename)
+  } catch (_) {
+    return ''
+  }
+}
+
 async function checkPullRequest(): Promise<boolean> {
   const pullPayload = github.context.payload as WebhookEventMap['pull_request']
   const pr = await github.getPullRequest(pullPayload.pull_request.number)
@@ -59,12 +68,18 @@ async function checkPullRequest(): Promise<boolean> {
   const filter = getIgnoreFilter()
 
   const files = await github.getPullRequestFiles(pr.number)
-  const jsFiles = files.filter((f) => isForbiddenJSFile(f.filename, filter))
-  const renamedJsFiles = files.filter(
+  const filesWithContent = await Promise.all(
+    files.map((f) => ({ ...f, fileContent: getFileContent(f.filename) }))
+  )
+  const forbiddenJsFiles = filesWithContent.filter((f, index) =>
+    isForbiddenJSFile(f.filename, f.fileContent, filter)
+  )
+
+  const renamedJsFiles = filesWithContent.filter(
     (f) =>
       f.previous_filename &&
       isForbiddenJSFile(f.previous_filename) &&
-      !jsFiles.includes(f)
+      !forbiddenJsFiles.includes(f)
   )
   const tsconfigFiles = await fs.glob({
     patterns: ['**/tsconfig.json'],
@@ -73,14 +88,14 @@ async function checkPullRequest(): Promise<boolean> {
 
   const errors: Error[] = []
 
-  if (jsFiles.length || tsconfigFiles.length) {
+  if (forbiddenJsFiles.length || tsconfigFiles.length) {
     errors.push(
-      ...(await checkJsUsage(jsFiles)),
+      ...(await checkJsUsage(forbiddenJsFiles)),
       ...(await checkTsConfig(tsconfigFiles))
     )
   }
 
-  if (jsFiles.length || renamedJsFiles.length || errors.length > 0) {
+  if (forbiddenJsFiles.length || renamedJsFiles.length || errors.length > 0) {
     const adoption = await measureTsAdoption(filter)
     const commentId = await github.pinComment(
       pr.number,
@@ -151,19 +166,26 @@ export async function measureTsAdoption(
     patterns: ['**/*.js', '**/*.jsx'],
     exclude: filter,
   })
+  const typedJsFiles = jsFiles.filter((file) => {
+    const fileContent = fs.readFile(file)
+    return !!fileContent.match(JS_TS_CHECK_COMMENT_REGEX)
+  })
   const tsFiles = await fs.glob({
     patterns: ['**/*.ts', '**/*.tsx'],
     exclude: filter,
   })
 
-  const jsLines = jsFiles
+  const untypedFiles = jsFiles.filter((f) => !typedJsFiles.includes(f))
+  const typedFiles = [...tsFiles, ...typedJsFiles]
+
+  const untypedLines = untypedFiles
     .map((f) => fs.readFile(f).split('\n').length)
     .reduce((total, lines) => total + lines, 0)
-  const tsLines = tsFiles
+  const typedLines = typedFiles
     .map((f) => fs.readFile(f).split('\n').length)
     .reduce((total, lines) => total + lines, 0)
 
-  return tsLines / (jsLines + tsLines)
+  return typedLines / (untypedLines + typedLines)
 }
 
 async function checkTsConfig(files: string[]): Promise<Error[]> {
@@ -201,11 +223,16 @@ async function checkTsConfig(files: string[]): Promise<Error[]> {
 
 export function isForbiddenJSFile(
   filename: string,
+  fileContent = '',
   filter: IgnoredFileFilter = getIgnoreFilter()
 ): boolean {
   const jsPattern = /\.jsx?$/i
+  const hasJsExtension = jsPattern.test(filename) && !filter.ignores(filename)
+  const appliesTypescriptViaComment = !!fileContent.match(
+    JS_TS_CHECK_COMMENT_REGEX
+  )
 
-  return jsPattern.test(filename) && !filter.ignores(filename)
+  return hasJsExtension && !appliesTypescriptViaComment
 }
 
 export function missingTsConfigSettings(tsconfig: TsConfig): string[] {