Snapchat / ts-inject

Typesafe dependency injection framework for TypeScript projects, providing easy-to-use, maintainable, and scalable code with strong type safety.
https://snapchat.github.io/ts-inject/
MIT License
5 stars 2 forks source link

Reconsider how overrides work #5

Open mikalai-snap opened 2 months ago

mikalai-snap commented 2 months ago

Problem Description

The core issue is that once a service is resolved in a container, it gets locked to the value that was provided at the time of resolution. When child containers override that value, the previously resolved service in the parent container still uses the old value, ignoring the override in the child. This results in unexpected behavior when working with container hierarchies.

Consider the following example:

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], (value: number) => value)
);

const child = parent.providesValue("value", 2);
expect(child.get("service")).toBe(2); // OK!

Here, the expectation is that the overridden value (2) is used by service in the child container. However, if we trigger the factory functions before providing the override, the behavior changes:

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], (value: number) => value)
);
parent.get("service"); // trigger container factory functions
const child = parent.providesValue("value", 2);
expect(child.get("service")).toBe(2); // FAIL! The value is 1

Once service is resolved in the parent, the child container inherits the cached version, which is locked to value: 1. This leads to confusing and inconsistent behavior, especially when working with complex container hierarchies.

However, the library already provides a solution for this issue through the Container.copy() method. This method allows you to scope specific services to the child container, ensuring that those services are re-instantiated with the new values in the child. For example:

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], (value: number) => value)
);
parent.get("service"); // trigger container factory functions
const child = parent.copy(["service"]).providesValue("value", 2);
expect(child.get("service")).toBe(2); // OK!

While Container.copy() allows scoping services to child containers, it relies on developers remembering to use it, which can introduce complexity and potential errors. It can feel excessive for simple value overrides, leading to unnecessary duplication of services and inefficiencies when only the value, not the service instance, needs to change.

What Other DI Libraries Do

Possible Solutions

This issue could be addressed at the API level. Below are several potential options:

1. Keep the Old Behavior

Maintain the current behavior, where services are locked to the value at the time of resolution. The downside is that this can lead to unpredictable container behavior, making it harder to track down bugs.

Pros:

2. Adopt Typed-Inject Behavior

Lock the service to the value that was provided at the time of its initial registration. This avoids any confusion caused by value overrides in child containers.

Pros:

3. Allow Explicit Control Over Reevaluation (My Preferred Solution)

Introduce overrides* methods that allow service overrides with explicit control over whether dependencies should be reevaluated. Developers would pass named constants such as "reevaluate" or "retain" to indicate whether the dependent services should be updated or left untouched.

Default Behavior: The default behavior should be to reevaluate dependencies when a value is overridden, as this aligns with the most intuitive expectation—overriding a value should update dependent services. If a developer explicitly wants to retain the old behavior, they can pass "retain" to prevent reevaluation.

Another option is to keep provides*() methods, but require developers to provide "reevaluate" | "retain" value if the service is already registered. If they try to use provides*() methods for already registered services without the scoping switch, they will get a compilation (and runtime) error.

Example:

   const parent = Container.providesValue("value", 1).provides(
     Injectable("service", ["value"], (value: number) => value)
   );

   parent.get("service"); // Triggers resolution of "service" with value 1

   // In the child container, override the "value" and specify to reevaluate dependencies
   const child = parent.overridesValue("value", 2, "reevaluate");
   //. const child = parent.providesValue("value", 2, "reevaluate"); // alternative
   expect(child.get("service")).toBe(2); // Reevaluates "service" with the new value 2

In this example, "reevaluate" is passed to ensure that services depending on the overridden "value" are reevaluated.

Example with No Reevaluation:

   const child = parent.overridesValue("value", 2, "retain");
   expect(child.get("service")).toBe(1); // Still returns the service resolved with the original value 1

Pros:

4. Forbid Service Overriding Entirely

The simplest solution is to forbid service overrides entirely. This would avoid ambiguity but also limits flexibility.

Pros:

5. Introduce a "Sealed" State (Statically Typed Language Inspiration)

Introduce a "sealed" state for containers. Until a container is sealed, users can provide new values and services. Once sealed, the container cannot be modified, but services can be resolved. Child containers could be created from sealed containers but would not inherit any resolved services.

   const c1Unsealed = Container.providesValue("v", 1).providesFactory("f", ["v"] as const, (v) => v);
   // Services cannot be resolved in unsealed containers
   // c1Unsealed.get("f"); // This will fail

   const c1 = c1Unsealed.seal();
   // Now services can be resolved
   expect(c1.get("f")).toBe(1);

   const c2 = Container.fromSealed(c1).providesValue("v", 2).seal();
   expect(c2.get("f")).toBe(2);

Pros:

Trade-Offs Between Solutions

Solution Pros Cons
1. Keep Old Behavior and Use copy No breaking changes, explicit scoping Requires users to remember to use copy, prone to human error
2. Typed-Inject Behavior Predictable, always tied to registration values Ignores user intent, breaking change
3. Explicit Control Over Reevaluation Flexible, user intent explicit, backward-compatible API complexity, cognitive load
4. Forbid Service Overriding Clear design patterns, simpler containers Inflexible, users may need overrides
5. Introduce "Sealed" State Predictable lifecycle, avoids caching issues Significant API change, more complexity

Conclusion

By allowing explicit control over reevaluation (option 3), we strike a balance between flexibility and predictability. Users can specify their intent clearly, and it minimizes ambiguity without introducing significant breaking changes. However, for existing users, leveraging the copy method (option 1) offers an immediate, backward-compatible solution that can address the issue today.

wfribley commented 2 months ago

This is such an awesome write-up!

First, I wanted to mention a requirement that may be implicit here, but may be worth making explicit:

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], (value: number) => value)
);

const child = parent.providesValue("value", 2);

// Containers are immutable, so even though we are invoking the "service" factory function
// *after* overriding "value", it should still use "value" from the parent container.

expect(parent.get("service")).toBe(1); // <- resolving "service" in the parent container

The value of any service's dependencies must only come from ancestor containers, never from descendent containers.

This may already be the behavior, just wanted to make sure this requirement is captured.


I like option 3 but wonder if it can be simplified + continue to use an (improved) Container.copy() to achieve the most natural behavior -- so I'll propose:

6. Always re-evaluate + explicitly freeze services that shouldn't be re-evaluated.

  1. When a Service is provided, it will always have the "reevaluate" behavior. In other words: when a Container resolves a Service, that Service will be provided the nearest-ancestor implementation of its dependencies even if that requires re-evaluating the Service's factory function.
  2. Authors can use Container.copy() to specify Services that are frozen and never re-evaluated. And it should probably have a better name...

This could look something like

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], (value: number) => value)
);

// We can resolve here or not, either way the following behavior will be the same.
expect(parent.get("service")).toBe(1)

const reevaluatedChild = parent.providesValue("value", 2);
expect(reevaluatedChild.get("service")).toBe(2);

// Rename "copy" to "freeze" and invert its behavior: the listed services are the only ones
// that are *not* re-scoped to the child.
const retainedChild = parent.freeze(["service"]).providesValue("value", 2);
expect(retainedChild.get("service")).toBe(1);

With re-evaluation being the default, I think Service authors will also need the ability to specify that their Service should not be re-evaluated (e.g. if the Service is expensive to create, or is designed to be a singleton):

const parent = Container.providesValue("value", 1).provides(
  Injectable("service", ["value"], freeze((value: number) => value)) // <- Service author freezes the service
);

const child = parent.providesValue("value", 2);
expect(child.get("service")).toBe(1); // <- "service" is not re-evaluated because its author froze it.

I think this has the same pros/cons of option 3, more or less, with a couple additional pros:

Note: other naming options I thought about for Container.freeze():


That's my suggestion, but I obviously don't have as much insight as you do into how big of a breaking change this might be for package consumers, or if this suggestion would adequately address existing pain points. Just throwin' it out there for consideration.

mikalai-snap commented 2 weeks ago

@wfribley, thanks for your feedback! I agree that having "always re-evaluate" as the default behavior seems most intuitive. I also like the idea of making services singletons when registering them. Just to clarify, when a service is frozen, there's no way to override it afterward, right?

Regarding the differences between Option 3 (Container.providesValue(value, "retain")) and Option 6 (Container.freeze()), there's a significant distinction:

  1. Container.providesValue(value, "retain") is applied when the override is registered. Here, the person overriding the value decides whether dependent services need to be re-evaluated.
  2. Container.freeze() is applied after the service is registered. In this case, the user of the container decides that any future overrides to dependencies won't affect the service.

I think both options are valid, but I feel that Container.freeze() is more practical for real-world use cases, but I'm not 100% sure. Like it is hard for me to follow developer though process: how do they know they need to use Container.freeze()? Say all singletons are already decorated with freeze(fn), what will make developers want to use Container.freeze()?..

I feel plugins is something where that can be applied. Therefore would like to get input from @kburov-sc: Given the plugin-based approach of the project you're working on, do you see an application for Container.freeze()? Any feedback in general regarding the issue?

wfribley commented 2 weeks ago

Just to clarify, when a service is frozen, there's no way to override it afterward, right?

I don't think so... but if a use-case arises, there could certainly be an API to support it.

Regarding the differences between Option 3 (Container.providesValue(value, "retain")) and Option 6 (Container.freeze()), there's a significant distinction:

  1. Container.providesValue(value, "retain") is applied when the override is registered. Here, the person overriding the value decides whether dependent services need to be re-evaluated.
  2. Container.freeze() is applied after the service is registered. In this case, the user of the container decides that any future overrides to dependencies won't affect the service.

Ah, yeah, that's true. To get the same behavior with Container.freeze(), you'd have to know which services depend on the override and freeze them before providing the override.


And yeah, it would be super useful to collect some real-world use-cases.