Code review changes!

Author: timfish

packages/node/src/integrations/contextlines.ts

@@ -53,28 +53,32 @@ export class ContextLines implements Integration {
    * @inheritDoc
    */
   public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
-    if (this._options.frameContextLines == undefined) {
+    // This is only here to copy frameContextLines from init options if it hasn't
+    // been set via this integrations constructor.
+    //
+    // TODO: Remove on next major!
+    if (this._options.frameContextLines === undefined) {
       const initOptions = getCurrentHub().getClient<NodeClient>()?.getOptions();
       // eslint-disable-next-line deprecation/deprecation
       this._options.frameContextLines = initOptions?.frameContextLines;
     }
 
-    addGlobalEventProcessor(event => this.addToEvent(event));
-  }
-
-  /** Processes an event and adds context lines */
-  public async addToEvent(event: Event): Promise<Event> {
     const contextLines =
       this._options.frameContextLines !== undefined ? this._options.frameContextLines : DEFAULT_LINES_OF_CONTEXT;
 
+    addGlobalEventProcessor(event => this.addSourceContext(event, contextLines));
+  }
+
+  /** Processes an event and adds context lines */
+  public async addSourceContext(event: Event, contextLines: number): Promise<Event> {
     const frames = event.exception?.values?.[0].stacktrace?.frames;
 
     if (frames && contextLines > 0) {
-      const filenames: string[] = [];
+      const filenames: Set<string> = new Set();
 
       for (const frame of frames) {
-        if (frame.filename && !filenames.includes(frame.filename)) {
-          filenames.push(frame.filename);
+        if (frame.filename) {
+          filenames.add(frame.filename);
         }
       }
 
@@ -91,8 +95,6 @@ export class ContextLines implements Integration {
           }
         }
       }
-
-      return event;
     }
 
     return event;
@@ -104,25 +106,20 @@ export class ContextLines implements Integration {
  *
  * @param filenames Array of filepaths to read content from.
  */
-async function readSourceFiles(filenames: string[]): Promise<Record<string, string | null>> {
-  // we're relying on filenames being de-duped already
-  if (!filenames.length) {
-    return {};
-  }
-
+async function readSourceFiles(filenames: Set<string>): Promise<Record<string, string | null>> {
   const sourceFiles: Record<string, string | null> = {};
 
   for (const filename of filenames) {
-    const cache = FILE_CONTENT_CACHE.get(filename);
+    const cachedFile = FILE_CONTENT_CACHE.get(filename);
     // We have a cache hit
-    if (cache !== undefined) {
+    if (cachedFile !== undefined) {
       // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip.
-      if (cache === null) {
+      if (cachedFile === null) {
         continue;
       }
 
       // Otherwise content is there, so reuse cached value.
-      sourceFiles[filename] = cache;
+      sourceFiles[filename] = cachedFile;
       continue;
     }
 

packages/node/src/sdk.ts

@@ -12,6 +12,7 @@ export const defaultIntegrations = [
   // Common
   new CoreIntegrations.InboundFilters(),
   new CoreIntegrations.FunctionToString(),
+  new ContextLines(),
   // Native Wrappers
   new Console(),
   new Http(),
@@ -20,8 +21,6 @@ export const defaultIntegrations = [
   new OnUnhandledRejection(),
   // Misc
   new LinkedErrors(),
-  // Context Lines
-  new ContextLines(),
 ];
 
 /**

packages/node/src/types.ts

@@ -23,7 +23,7 @@ export interface NodeOptions extends Options {
   /**
    * Sets the number of context lines for each frame when loading a file.
    *
-   * @deprecated Context lines configuration has moved to the `ContextLines` integration.
+   * @deprecated Context lines configuration has moved to the `ContextLines` integration, and can be used like this:
    *
    * ```
    * init({

packages/node/test/parsers.test.ts

@@ -8,15 +8,15 @@ import { getError } from './helper/error';
 
 describe('parsers.ts', () => {
   let frames: stacktrace.StackFrame[];
-  let spy: jest.SpyInstance;
+  let readFileSpy: jest.SpyInstance;
   let contextLines: ContextLines;
 
-  async function addContext(frames: StackFrame[]): Promise<void> {
-    await contextLines.addToEvent({ exception: { values: [{ stacktrace: { frames } }] } });
+  async function addContext(frames: StackFrame[], lines: number = 7): Promise<void> {
+    await contextLines.addSourceContext({ exception: { values: [{ stacktrace: { frames } }] } }, lines);
   }
 
   beforeEach(() => {
-    spy = jest.spyOn(fs, 'readFile');
+    readFileSpy = jest.spyOn(fs, 'readFile');
     frames = stacktrace.parse(new Error('test'));
     contextLines = new ContextLines();
     resetFileContentCache();
@@ -30,16 +30,16 @@ describe('parsers.ts', () => {
     test('parseStack with same file', async () => {
       expect.assertions(1);
 
-      let mockCalls = 0;
       let parsedFrames = Parsers.parseStack(frames);
       await addContext(parsedFrames);
 
-      mockCalls = spy.mock.calls.length;
+      const numCalls = readFileSpy.mock.calls.length;
       parsedFrames = Parsers.parseStack(frames);
       await addContext(parsedFrames);
 
-      // Calls to readFile shouldn't increase if there isn't a new error
-      expect(spy).toHaveBeenCalledTimes(mockCalls);
+      // Calls to `readFile` shouldn't increase if there isn't a new error to
+      // parse whose stacktrace contains a file we haven't yet seen
+      expect(readFileSpy).toHaveBeenCalledTimes(numCalls);
     });
 
     test('parseStack with ESM module names', async () => {
@@ -58,27 +58,20 @@ describe('parsers.ts', () => {
       ];
       const parsedFrames = Parsers.parseStack(framesWithFilePath);
       await addContext(parsedFrames);
-      expect(spy).toHaveBeenCalledTimes(1);
+      expect(readFileSpy).toHaveBeenCalledTimes(1);
     });
 
     test('parseStack with adding different file', async () => {
-      expect.assertions(2);
-      let mockCalls = 0;
-      let newErrorCalls = 0;
+      expect.assertions(1);
       let parsedFrames = Parsers.parseStack(frames);
       await addContext(parsedFrames);
 
-      mockCalls = spy.mock.calls.length;
+      const numCalls = readFileSpy.mock.calls.length;
       parsedFrames = Parsers.parseStack(stacktrace.parse(getError()));
       await addContext(parsedFrames);
 
-      newErrorCalls = spy.mock.calls.length;
-      expect(newErrorCalls).toBeGreaterThan(mockCalls);
-
-      parsedFrames = Parsers.parseStack(stacktrace.parse(getError()));
-      await addContext(parsedFrames);
-
-      expect(spy).toHaveBeenCalledTimes(newErrorCalls);
+      const newErrorCalls = readFileSpy.mock.calls.length;
+      expect(newErrorCalls).toBeGreaterThan(numCalls);
     });
 
     test('parseStack with duplicate files', async () => {
@@ -115,16 +108,14 @@ describe('parsers.ts', () => {
 
       const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles);
       await addContext(parsedFrames);
-      expect(spy).toHaveBeenCalledTimes(1);
+      expect(readFileSpy).toHaveBeenCalledTimes(1);
     });
 
     test('parseStack with no context', async () => {
-      contextLines = new ContextLines({ frameContextLines: 0 });
-
       expect.assertions(1);
       const parsedFrames = Parsers.parseStack(frames);
-      await addContext(parsedFrames);
-      expect(spy).toHaveBeenCalledTimes(0);
+      await addContext(parsedFrames, 0);
+      expect(readFileSpy).toHaveBeenCalledTimes(0);
     });
   });
 });