Draft: convert file get-user-code-frame.js to TypeScript

[lindapaiste]

Continuing conversion to TypeScript per issue #494.

Checklist:

• Documentation added to the
  docs site
• Tests
• Typescript definitions updated
• Ready to be merged

---

The types for the get-user-code-frame.js file are raising enough issues that I'm making a PR just for this one file.

Adding TypeScript types highlights potential issues in the code itself. In my opinion there are a few potential issues here where the TypeScript warnings are correct and should not be ignored. So there are some things that I'd like to change.

Don't Understand: const nodeRequire

There are other parts that I don't understand fully, and I might be missing something. Like this:

try {
  const nodeRequire: NodeJS.Require = module && module.require
  
  readFileSync = nodeRequire.call(module, 'fs').readFileSync

I'm unsure of the reasons for:

• assigning module.require to a variable
• using module.require vs. require
• using call syntax

It's inside a try/catch so it seems like it should be okay to just call module.require('fs') directly and let any errors be thrown.

Can Improve: potentially undefined firstClientCodeFrame

function getUserCodeFrame() {
  // If we couldn't load dependencies, we can't generate the user trace
  /* istanbul ignore next */
  if (!readFileSync || !codeFrameColumns) {
    return ''
  }
  const err = new Error()
  const firstClientCodeFrame = err.stack
    .split('\n')
    .slice(1) // Remove first line which has the form "Error: TypeError"
    .find(frame => !frame.includes('node_modules/')) // Ignore frames from 3rd party libraries

  return getCodeFrame(firstClientCodeFrame)
}

const firstClientCodeFrame has possible undefined issues in two places: The Error is not guaranteed to have a stack property and find is not guaranteed to find a match.

Can Improve: potentially invalid imports

The current code assumes that if readFileSync is non-null then chalk must be as well. Do we know that for certain? The call to chalk.dim in getCodeFrame is not located inside of a try/catch block.

Solution 1) Move the try/catch higher up so that it encloses the entire function body.

Solution 2) Allow the getCodeFrame function to throw any Errors and catch them when it is called in getUserCodeFrame. The only export from this file is getUserCodeFrame, so changing the behaviors of other functions or variables is not a problem.

Solution 3) Enforce that the initial try/catch block which assigns the variables must have all variables or none. We can do this by returning an object with three properties instead of using three independent variables.

Example of solution 3 implementation using IIFE:

interface NodeFns {
  chalk: Chalk
  readFileSync: typeof ReadFileSyncFn
  codeFrameColumns: typeof CodeFrameColumnsFn
}

const nodeFns: NodeFns | null = (() => {
  try {
    return {
      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-member-access
      readFileSync: module.require('fs').readFileSync,
      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-member-access
      codeFrameColumns: module.require('@babel/code-frame').codeFrameColumns,
      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
      chalk: module.require('chalk'),
    }
  } catch {
    // We're in a browser environment
    return null
  }
})()

But my inclination is towards Solution 2, which is what I did in the PR.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a199cdf:

Sandbox	Source
react-testing-library-examples	Configuration