Some refactoring of ResolutionSession that makes module implementation very semantic and concise

[thephoenixofthevoid]

The main point of this PR is changes in packages/context/src/resolution-session.ts.

The original code contains some complexity:

1. Internal method enterBinding violates "the single concern" principle: it ensures that ResolutionSession exists or created, and also it is concerned with tracking resolution status.
2. The code failed to express the intent that entering and leaving binding must happen just before action started and right after it is done.
3. The same thing happens with injection-related methods

So I managed to get rid of private method, and without changing public interface managed to rewrite runWith- methods in sane way. Now runWithBinding looks this way:

class  ResolutionSession {
  // .....skipped....
  static runWithBinding(
    action: ResolutionAction,
    binding: Readonly<Binding>,
    session = new ResolutionSession(),
  ) {
    // Start to resolve a binding within the session
    session.pushBinding(binding);
    return tryWithFinally(
      () => action(session),
      () => session.popBinding(),
    );
  }
  // .....skipped....
}

Here we can see the important invariant for order of invocation of pushBinding and popBinding: push and pop calls must be balanced and the state of stack before push must be restored after pop call. Now "opening" and "closing" happened to end up in the same function, and there are no other places where push/pop methods are invoked. That means that we must treat pushBinding/popBinding/pushInjection/popInjection as private.

That itself renders checks inside these methods useless, effectively turning them (since these two kinds do not differ at all if checks and debugging are removed) into a single pair of push and pop methods. So there are no longer need for keeping different runWithBinding/runWithInjection. Now we may have:

class  ResolutionSession {
  // .....skipped....
  static runWith<T>(action: ResolutionAction, value: Readonly<T>,  session = new ResolutionSession()) {
    session.stack.push(value);
    return tryWithFinally(
      () => action(session),
      () => assert(session.stack.pop() === value),
    );
  }

  // .....skipped....

  // pushBinding may be deleted
  // popBinding  may be deleted
  // pushInjection may be deleted
  // popInjection may be deleted

  // .....skipped....
}

There are 2 more changes, but these are more of housekeeping type of changes.

Would like to hear your opinion on that and whether you are interested in me working in that direction.

Regards.

Checklist

• [+] DCO (Developer Certificate of Origin) signed in all commits
• [+] npm test passes on your machine
• [?] New tests added or existing tests modified to cover all changes
• [?] Code conforms with the style guide
• [?] API Documentation in code was updated
• [?] Documentation in /docs/site was updated
• [?] Affected artifact templates in packages/cli were updated
• [?] Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[thephoenixofthevoid]

Also, I just realize it, instead of using an array to track resolution session state, and having to copy it at fork and things like that, we may consider the following class (let me use # to represent private properties -- just for the sake of brevity):

class ResolutionSession {

  #value?: ResolutionElement = undefined;
  #parent?:  ResolutionSession = undefined;
  
  constructor(parent?:  ResolutionSession, value?: ResolutionElement) {
      // parent and value must be defined both or both undefined (ROOT)
      this.#parent = parent;
      this.#value = value;
  }

  // We actually don't need fork anymore, because ResolutionSession 
  // is created for each single invocation of ResolutionAction, storing ptr to the parent
  // but parent intentionally does not track children. Instead of popping from array we simply 
  // forget reference to the lowest accessible instance of ResolutionSession.
  fork() {
      return new ResolutionSession(
           this.#parent, 
           this.#value
      )
  }
 // UPD: I looked all cases where fork is used in current codebase. It's primary 
 //           goal to counter-play the changing stack. But with this implementation
 //           we simply pass  instance of ResolutionSession and it's perfectly fine
 //          
 // It seems like a way to completely get rid of fragility in this module.
 // I may create a proof of concept (a drop-in replacement of existing class), if you want


  runWith(value: ResolutionElement, action: ResolutionAction) {
     return action(
         new ResolutionSession(this, value)
     )
  }

  get stack() { // Called so to show how props from forming implementation can be extracted 
      var current = this;
      var list = [];
      while (current && current.#value) {
         list.push(current.#value)
         current = current.#parent
      }
      list.reverse()
      return  list;
  }

  // All other methods can be implemented using .stack getter
}

[raymondfeng]

Not sure if these changes are relevant.

[thephoenixofthevoid]

These are not. I may undo what can be considered a style change, if you want.

[raymondfeng]

Let's revert it for this PR.

[thephoenixofthevoid]

Just done it. Reverted.

[thephoenixofthevoid]

Also, I just realize it, instead of using an array to track resolution session state, and having to copy it at fork and things like that, we may consider the following class (let me use # to represent private properties -- just for the sake of brevity):

class ResolutionSession {

  #value?: ResolutionElement = undefined;
  #parent?:  ResolutionSession = undefined;
  
  constructor(parent?:  ResolutionSession, value?: ResolutionElement) {
      // parent and value must be defined both or both undefined (ROOT)
      this.#parent = parent;
      this.#value = value;
  }

  // We actually don't need fork anymore, because ResolutionSession 
  // is created for each single invocation of ResolutionAction, storing ptr to the parent
  // but parent intentionally does not track children. Instead of popping from array we simply 
  // forget reference to the lowest accessible instance of ResolutionSession.
  fork() {
      return new ResolutionSession(
           this.#parent, 
           this.#value
      )
  }
 // UPD: I looked all cases where fork is used in current codebase. It's primary 
 //           goal to counter-play the changing stack. But with this implementation
 //           we simply pass  instance of ResolutionSession and it's perfectly fine
 //          
 // It seems like a way to completely get rid of fragility in this module.
 // I may create a proof of concept (a drop-in replacement of existing class), if you want


  runWith(value: ResolutionElement, action: ResolutionAction) {
     return action(
         new ResolutionSession(this, value)
     )
  }

  get stack() { // Called so to show how props from forming implementation can be extracted 
      var current = this;
      var list = [];
      while (current && current.#value) {
         list.push(current.#value)
         current = current.#parent
      }
      list.reverse()
      return  list;
  }

  // All other methods can be implemented using .stack getter
}

I did a Proof of Concept here in a separate branch:
https://github.com/thephoenixofthevoid/loopback-next/blob/f40c957c6f80129ba2b7036d35095ed7efe142c2/packages/context/src/resolution-session.ts
Note that fork function is just returning session without cloning, because we no longer need cloning in this implementation. It exists because other modules expect it to be. If this implementation ever gets into upstream, code that calls .fork() can be simplified even further

This implementation also uncovered the actual semantics for what this module seeks to achieve (by implementing it in simpler terms with fewer concepts). That is: we want modified session to be accessable solely inside actions, and nested actions. Instead of tracking starts and ends of actions, this implementation simply creates a child session for that action.

[raymondfeng]

The change LGTM. Would you please squash all commits into one?

[thephoenixofthevoid]

The change LGTM. Would you please squash all commits into one?

Just did that. Note small changes in test to accommodate using single describe function. This can be reverted too though.

[raymondfeng]

Let's not break the current injection path. Please revert the @ prefix.

[thephoenixofthevoid]

Updated

[thephoenixofthevoid]

Should I wait until it gets merged before discussing more changes?

[raymondfeng]

@thephoenixofthevoid Thank you for the contribution. Please open new PRs for follow-up enhancements.