Separate object creation from non-trivial operations.

[ericsnowcurrently]

Constructors really shouldn't do anything more than trivial setting of state. However, we have quite a few classes with non-trivial constructors. (For example, of those with @injectable applied, there are at least 96. See #10454.) This adds complexity that makes it harder to understand what is happening when the extension is starting up, much less carefully controlling and adapting activation.

To address this, we should do the following:

• move all non-trivial initialization out of constructors
• split up object init into several distinct phases

The phases would be something like this:

1. creation
   • provided with dependencies (which it must treat as un-initialized) and POTS values
   • stores them for use in later phases
   • sets up and stores trivial immutable state
   • (minimally) sets up and stores trivial initial mutable state
   • more complex init at the phase should be done instead in a factory function
2. simple-init
   • relies only on stored dependencies and initial state
   • stored dependencies are treated as simply-initialized (forced?)
   • idempotent (for mutable state, short-circuit)
   • only trivial work
   • sets up and stores all remaining trivial initial mutable state
   • not async
3. complex-init
   • relies only on state (incl. stored dependencies)
   • stored dependencies are treated as fully initialized (forced?)
   • does non-trivial work to finish setting up state
   • interaction with external resources (e.g. IO, telemetry)
   • async operations
   • if it relies on having some callback or event handler set then either this should block until that happens or it should make a call on a dependency (e.g. a wrapper func) to force it to happen
   • at this point it is ready to be used
4. start any background operations
   • relies only on state (incl. stored dependencies)
   • stored dependencies are treated as fully initialized (forced?)
   • includes starting "daemon" processes
5. registration
   • relies only on state (incl. stored dependencies)
   • registers callbacks and event handlers
   • at this point we should expect it be fully used

A hard separation between these phases makes it much easier to understand what is going on and to divide up extension activation along horizontal layers. Note that the phases easily map onto methods. An interface (for discussion purposes only) could look like one of the following:

interface Initializable {
    // 1
    constructor(...);
    // 2
    initializeSimple();
    // 3
    initialize(): Promise<void>;
    // 4
    start(): Promise<void>;
    // 5
    register();
}

interface Initializable {
    // 1 & 2 (if we are a little more permissive about what's done in the constructor)
    constructor(...);
    // 3
    initialize(): Promise<void>;
    // 4
    start(): Promise<void>;
    // 5
    register();
}

interface Initializable {
    // 1
    constructor(...);
    // 2
    initialize();
    // 3, 4, & 5
    activate(): Promise<void>;
}

(Note that the problem has been addressed for some classes, though with various different solutions. So this is also the right time to apply a consistent approach to how we tackle the problem.)

Of particular interest are classes with any of the following characteristics:

• with @injectable applied:
  • has mutable state
  • registers a callback or event handler
• all:
  • constructor with non-empty body, noting trivial vs. non-trivial
    • non-trivial: performs work and/or interacts with external resources
    • setting up simple state is trivial
  • has one of the following methods: initialize(), register*(), activate(), start()

Relative to those classes, we should consolidate on a simple initialization "interface" that we can use consistently during extension activation. It should also make it easy to identify the level of complexity of a class's initialization.

[ericsnowcurrently]

In terms of concrete steps, the following should be done for all classes in the code base, with an initial preference toward those with @injectable applied:

1. identify every class that need attention and add a TODO to each referencing this issue
2. eliminate any existing initialize() and register*() methods
   • move all trivial init code to the constructor (phases 1 & 2)
     • trivial init of immutable object state
     • trivial init of mutable object state
     • nothing async
     • treat provided args (dependencies) as though they have not been initialized yet
   • move all other code to an activate() method (phases 3, 4, & 5)
     • consistent signature: public async activate(): Promise<void>
     • non-trivial work (including complex init of mutable object state)
     • interaction with external resources (e.g. IO, telemetry)
     • any async operations
     • registering a callback or event handler
3. update activateLegacy() (in src/client/extensionActivation.ts) to call the various activate() methods in the right locations

It may make sense to be a bit more granular about it:

1. ...
2. move any callback or event handler registration into a register() method (phase 5)
   • consistent signature: public register(): void
   • assumes that the object is fully initialized
   • assumes that any necessary background operations have been started
3. move any starting of background operations to a start() method (phase 4)
   • consistent signature: public async start(): Promise<void>
4. eliminate any activate() method
   • move any init code to an initialize() method (phase 3)
     • consistent signature: public async initialize(): Promise<void>
   • move any activation-time telemetry code activateLegacy() (in src/client/extensionActivation.ts)
   • any other code (there shouldn't be any) should be moved to registered callback handlers
5. move to the constructor any trivial init of object state in initialize() (phases 1 & 2)
6. update activateLegacy() (in src/client/extensionActivation.ts) to call the various initialize(), start(), and register() methods in the right locations

Splitting phases 1 & 2 could happen as well, but that isn't as important.

---

There is also the question of how to make sure we do not regress. It may make sense to add a test that verifies that all constructor bodies are empty. That should be doable without much parsing complexity. It would require that all classes be updated to move everything out of the constructor body and into an initialize() method. However, this might not be practical (e.g. super() calls). It could make sense to look for a marker comment at the beginning of a non-empty constructor indicating that it has been manually vetted.

[ericsnowcurrently]

We'll try this out with the upcoming "env discovery" work.

[karrtikr]

We did this: #10542 (comment), closing.