refactor: move to inversify for dependency injection (microsoft#316)

* refactor: move to inversify for dependency injection Following microsoft#312, we realized service wiring was getting rather messy and burdensome. This PR brings Inversify in to provide dependency injection. Inversify support hierarchal containers, which works nicely for our case where we have global services (e.g. delegate launcher), top-level session services (e.g. the logger and launchers) as well as session-specific services. This PR mostly just moves the services that were previously in our ad-hoc services.ts into Inversify. It's my plan that as we touch things and expand on code, we can move more services into the container. * fixup! tests, config for running in vs
relief-melone · Feb 10, 2020 · 7de2c6b · 7de2c6b
1 parent 4170dc5
commit 7de2c6b
Show file tree

Hide file tree

Showing 44 changed files with 482 additions and 340 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -51,9 +51,11 @@
     "@c4312/chromehash": "^0.2.0",
     "color": "^3.1.2",
     "glob-stream": "^6.1.0",
+    "inversify": "^5.0.1",
     "js-beautify": "^1.10.0",
     "jsonc-parser": "^2.2.0",
     "micromatch": "^4.0.2",
+    "reflect-metadata": "^0.1.13",
     "source-map": "^0.7.3",
     "split2": "^3.1.1",
     "typescript": "^3.7.4",

diff --git a/src/adapter/breakpointPredictor.ts b/src/adapter/breakpointPredictor.ts
@@ -14,8 +14,9 @@ import { CorrelatedCache } from '../common/sourceMaps/mtimeCorrelatedCache';
 import { LogTag, ILogger } from '../common/logging';
 import { AnyLaunchConfiguration } from '../configuration';
 import { EventEmitter } from '../common/events';
-import { SourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
+import { ISourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
 import { logPerf } from '../telemetry/performance';
+import { injectable, inject } from 'inversify';
 
 export interface IWorkspaceLocation {
   absolutePath: string;
@@ -33,32 +34,62 @@ type MetadataMap = Map<string, Set<DiscoveredMetadata>>;
 
 const longPredictionWarning = 10 * 1000;
 
-export type BreakpointPredictionCache = CorrelatedCache<number, DiscoveredMetadata[]>;
+export const IBreakpointsPredictor = Symbol('IBreakpointsPredictor');
 
-export class BreakpointsPredictor {
+/**
+ * Determines ahead of time where to set breakpoints in the target files
+ * by looking at source maps on disk.
+ */
+export interface IBreakpointsPredictor {
+  /**
+   * Returns a promise that resolves once maps in the root are predicted.
+   */
+  prepareToPredict(): Promise<void>;
+
+  /**
+   * Returns a promise that resolves when breakpoints for the given location
+   * are predicted.
+   */
+  predictBreakpoints(params: Dap.SetBreakpointsParams): Promise<void>;
+
+  /**
+   * Returns predicted breakpoint locations for the provided source.
+   */
+  predictedResolvedLocations(location: IWorkspaceLocation): IWorkspaceLocation[];
+}
+
+@injectable()
+export class BreakpointsPredictor implements IBreakpointsPredictor {
   private readonly predictedLocations: PredictedLocation[] = [];
   private readonly patterns: string[];
   private readonly rootPath: string;
   private readonly longParseEmitter = new EventEmitter<void>();
   private sourcePathToCompiled?: Promise<MetadataMap>;
+  private cache?: CorrelatedCache<number, DiscoveredMetadata[]>;
 
   /**
    * Event that fires if it takes a long time to predict sourcemaps.
    */
   public readonly onLongParse = this.longParseEmitter.event;
 
   constructor(
-    launchConfig: AnyLaunchConfiguration,
-    private readonly repo: ISourceMapRepository,
-    private readonly logger: ILogger,
-    private readonly sourceMapFactory: SourceMapFactory,
+    @inject(AnyLaunchConfiguration) launchConfig: AnyLaunchConfiguration,
+    @inject(ISourceMapRepository) private readonly repo: ISourceMapRepository,
+    @inject(ILogger) private readonly logger: ILogger,
+    @inject(ISourceMapFactory) private readonly sourceMapFactory: ISourceMapFactory,
+    @inject(ISourcePathResolver)
     private readonly sourcePathResolver: ISourcePathResolver | undefined,
-    private readonly cache: BreakpointPredictionCache | undefined,
   ) {
     this.rootPath = launchConfig.rootPath;
     this.patterns = launchConfig.outFiles.map(p =>
       path.isAbsolute(p) ? path.relative(launchConfig.rootPath, p) : p,
     );
+
+    if (launchConfig.__workspaceCachePath) {
+      this.cache = new CorrelatedCache(
+        path.join(launchConfig.__workspaceCachePath, 'bp-predict.json'),
+      );
+    }
   }
 
   private async createInitialMapping(): Promise<MetadataMap> {
@@ -129,7 +160,7 @@ export class BreakpointsPredictor {
   }
 
   /**
-   * Returns a promise that resolves once maps in the root are predicted.
+   * @inheritdoc
    */
   public async prepareToPredict(): Promise<void> {
     if (!this.sourcePathToCompiled) {

diff --git a/src/adapter/debugAdapter.ts b/src/adapter/debugAdapter.ts
@@ -16,18 +16,21 @@ import { ISourcePathResolver } from '../common/sourcePathResolver';
 import { AnyLaunchConfiguration } from '../configuration';
 import { TelemetryReporter } from '../telemetry/telemetryReporter';
 import { IDeferred, getDeferred } from '../common/promiseUtil';
-import { SourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
-import { ScriptSkipper } from './scriptSkipper';
+import { ISourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
+import { ScriptSkipper, IScriptSkipper } from './scriptSkipper';
 import { IAsyncStackPolicy } from './asyncStackPolicy';
-import { LogTag } from '../common/logging';
-import { DisposableList } from '../common/disposable';
-import { IServiceCollection } from '../services';
+import { LogTag, ILogger } from '../common/logging';
+import { DisposableList, IDisposable } from '../common/disposable';
+import { Container } from 'inversify';
+import { ISourceMapRepository } from '../common/sourceMaps/sourceMapRepository';
+import { IBreakpointsPredictor } from './breakpointPredictor';
+import { disposeContainer } from '../ioc-extras';
 
 const localize = nls.loadMessageBundle();
 
 // This class collects configuration issued before "launch" request,
 // to be applied after launch.
-export class DebugAdapter {
+export class DebugAdapter implements IDisposable {
   readonly dap: Dap.Api;
   readonly sourceContainer: SourceContainer;
   readonly breakpointManager: BreakpointManager;
@@ -41,11 +44,10 @@ export class DebugAdapter {
     dap: Dap.Api,
     rootPath: string | undefined,
     sourcePathResolver: ISourcePathResolver,
-    private readonly _scriptSkipper: ScriptSkipper,
     private readonly asyncStackPolicy: IAsyncStackPolicy,
     private readonly launchConfig: AnyLaunchConfiguration,
     private readonly _rawTelemetryReporter: TelemetryReporter,
-    private readonly _sharedServices: IServiceCollection,
+    private readonly _services: Container,
   ) {
     this._configurationDoneDeferred = getDeferred();
     this.dap = dap;
@@ -80,25 +82,22 @@ export class DebugAdapter {
       })),
     );
 
-    const sourceMapFactory = new SourceMapFactory(_sharedServices.logger);
-    this._disposables.push(sourceMapFactory);
-
     this.sourceContainer = new SourceContainer(
       this.dap,
-      sourceMapFactory,
-      _sharedServices.logger,
+      _services.get(ISourceMapFactory),
+      _services.get(ILogger),
       rootPath,
       sourcePathResolver,
-      _sharedServices.sourceMapRepo,
-      this._scriptSkipper,
+      _services.get(ISourceMapRepository),
+      _services.get(IScriptSkipper),
     );
-    this._scriptSkipper.setSourceContainer(this.sourceContainer);
+
     this.breakpointManager = new BreakpointManager(
       this.dap,
       this.sourceContainer,
-      _sharedServices.logger,
+      _services.get(ILogger),
       launchConfig.pauseForSourceMap,
-      _sharedServices.bpPredictor,
+      _services.get(IBreakpointsPredictor),
     );
 
     this._rawTelemetryReporter.onFlush(() => {
@@ -268,7 +267,7 @@ export class DebugAdapter {
       cdp,
       this.dap,
       delegate,
-      this._sharedServices.logger,
+      this._services.get(ILogger),
       this.launchConfig,
       this.breakpointManager,
     );
@@ -279,7 +278,9 @@ export class DebugAdapter {
       .connect(cdp)
       .then(d => this._disposables.push(d))
       .catch(err =>
-        this._sharedServices.logger.error(LogTag.Internal, 'Error enabling async stacks', err),
+        this._services
+          .get<ILogger>(ILogger)
+          .error(LogTag.Internal, 'Error enabling async stacks', err),
       );
 
     this._thread.setPauseOnExceptionsState(this._pauseOnExceptionsState);
@@ -314,7 +315,7 @@ export class DebugAdapter {
   async _toggleSkipFileStatus(
     params: Dap.ToggleSkipFileStatusParams,
   ): Promise<Dap.ToggleSkipFileStatusResult | Dap.Error> {
-    await this._scriptSkipper.toggleSkippingFile(params);
+    await this._services.get<ScriptSkipper>(IScriptSkipper).toggleSkippingFile(params);
     await this._refreshStackTrace();
     return {};
   }
@@ -365,5 +366,6 @@ export class DebugAdapter {
 
   dispose() {
     this._disposables.dispose();
+    disposeContainer(this._services);
   }
 }
diff --git a/src/adapter/scriptSkipper.ts b/src/adapter/scriptSkipper.ts
@@ -14,13 +14,19 @@ import * as urlUtils from '../common/urlUtils';
 import Dap from '../dap/api';
 import { ITarget } from '../targets/targets';
 import { Source, SourceContainer } from './sources';
+import { injectable, inject } from 'inversify';
+import { ICdpApi } from '../cdp/connection';
+import { AnyLaunchConfiguration } from '../configuration';
 
 interface ISharedSkipToggleEvent {
   rootTargetId: string;
   targetId: string;
   params: Dap.ToggleSkipFileStatusParams;
 }
 
+export const IScriptSkipper = Symbol('IScriptSkipper');
+
+@injectable()
 export class ScriptSkipper {
   private static sharedSkipsEmitter = new EventEmitter<ISharedSkipToggleEvent>();
 
@@ -48,10 +54,10 @@ export class ScriptSkipper {
   private _rootTargetId: string;
 
   constructor(
-    skipPatterns: ReadonlyArray<string>,
-    private readonly logger: ILogger,
-    private readonly cdp: Cdp.Api,
-    target: ITarget,
+    @inject(AnyLaunchConfiguration) { skipFiles }: AnyLaunchConfiguration,
+    @inject(ILogger) private readonly logger: ILogger,
+    @inject(ICdpApi) private readonly cdp: Cdp.Api,
+    @inject(ITarget) target: ITarget,
   ) {
     this._targetId = target.id();
     this._rootTargetId = getRootTarget(target).id();
@@ -60,8 +66,8 @@ export class ScriptSkipper {
       this._normalizeUrl(key),
     );
 
-    this._preprocessNodeInternals(skipPatterns);
-    this._setRegexForNonNodeInternals(skipPatterns);
+    this._preprocessNodeInternals(skipFiles);
+    this._setRegexForNonNodeInternals(skipFiles);
     this._initNodeInternals(target); // Purposely don't wait, no need to slow things down
     this._newScriptDebouncer = debounce(100, () => this._initializeSkippingValueForNewSources());
 

diff --git a/src/adapter/sources.ts b/src/adapter/sources.ts
@@ -16,7 +16,7 @@ import { SourceMapConsumer, Position, NullablePosition } from 'source-map';
 import { SourceMap } from '../common/sourceMaps/sourceMap';
 import { ISourceMapRepository } from '../common/sourceMaps/sourceMapRepository';
 import { MapUsingProjection } from '../common/datastructure/mapUsingProjection';
-import { SourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
+import { CachingSourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
 import { LogTag, ILogger } from '../common/logging';
 import Cdp from '../cdp/api';
 import { createHash } from 'crypto';
@@ -319,14 +319,15 @@ export class SourceContainer {
 
   constructor(
     dap: Dap.Api,
-    private readonly sourceMapFactory: SourceMapFactory,
+    private readonly sourceMapFactory: CachingSourceMapFactory,
     private readonly logger: ILogger,
     public readonly rootPath: string | undefined,
     public readonly sourcePathResolver: ISourcePathResolver,
     public readonly localSourceMaps: ISourceMapRepository,
     public readonly scriptSkipper: ScriptSkipper,
   ) {
     this._dap = dap;
+    scriptSkipper.setSourceContainer(this);
   }
 
   setSourceMapTimeouts(sourceMapTimeouts: SourceMapTimeouts) {