Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider how overrides work #5

Open
mikalai-snap opened this issue Sep 5, 2024 · 1 comment
Open

Reconsider how overrides work #5

mikalai-snap opened this issue Sep 5, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@mikalai-snap
Copy link
Collaborator

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

  • typed-inject: Seals the service to the value that was provided at the time of its registration. This means overrides in child containers are ignored once the service is resolved.
  • inversify: Throws an error when trying to register the same service twice. It provides a dedicated method (rebind) to explicitly override a service.

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:

  • No breaking changes.
  • Simple and consistent with the current system.

Cons:

  • Can result in unpredictable behavior with nested containers.
  • Difficult to debug and can lead to issues when overrides are expected to work.

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:

  • Consistent behavior for services, no ambiguity about what value is being used.

Cons:

  • Ignores the user's intent when providing overrides in child containers.
  • May introduce breaking changes, as it deviates from the current behavior.
  • Confusion may still exist, as users might expect child containers to override dependencies.

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:

  • Provides explicit control over whether to reevaluate dependent services.
  • Defaulting to reevaluation ensures consistency and avoids accidental use of stale values.
  • The API is more readable, using named constants like "reevaluate" and "retain" rather than relying on true or false.

Cons:

  • Adds a slight learning curve as developers need to understand when to use "reevaluate" vs "retain".
  • While the default behavior is intuitive, it might still introduce performance overhead in large applications where reevaluating services is costly.

4. Forbid Service Overriding Entirely

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

Pros:

  • Forces clear design patterns, preventing unintended side effects.
  • Simplifies container management by removing the possibility of value overrides.

Cons:

  • Lacks flexibility. Some users may need the ability to override services in child containers.
  • Could break existing functionality where overrides are currently used.

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:

  • Predictable, as services cannot be resolved until the container is sealed.
  • Avoids caching-related issues, as services are not locked before sealing.
  • Makes the container lifecycle more explicit and manageable.

Cons:

  • Significant API change.
  • Could introduce more rigidity into container usage.
  • Adds complexity by requiring containers to be sealed before use.

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.

@mikalai-snap mikalai-snap added the enhancement New feature or request label Sep 5, 2024
@wfribley
Copy link

wfribley commented Sep 6, 2024

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:

  • Usage is simpler, for the majority of cases the default behavior is what users will want, and they don't have to understand "reevaluate" vs "retain"
  • More flexibility -- it's not "re-evaluate all dependencies" or "retain all dependencies," instead users can choose which dependencies to freeze and which to re-evaluate.

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

  • parent.readonly(["service"]).providesValue("value", 2) or parent.withReadonly(...)
  • parent.const(["service"]).providesValue("value", 2) or parent.withConst(...)
  • parent.memoize(["service"]).providesValue("value", 2) (I don't really like this one because memoize implies that "service" will be re-evaluated if its dependencies change, which is exactly the opposite behavior from what this would do)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants