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

Add option to detect and error on dependency lifetime mismatch #349

Merged
merged 17 commits into from
Jan 7, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,16 @@ app.get('/messages', (req, res) => {
```

**IMPORTANT!** If a singleton is resolved, and it depends on a scoped or
transient registration, those will remain in the singleton for it's lifetime!
transient registration, those will remain in the singleton for its lifetime!
Similarly, if a scoped module is resolved, and it depends on a transient
registration, that remains in the scoped module for its lifetime.
In the example above, if `messageService` was a singleton, it would be cached
in the root container, and would always have the `currentUser` from the first
request. Modules should generally not have a longer lifetime than their
dependencies, as this can cause issues of stale data.

If you want a mismatched configuration like the above to error, set
`errorOnShorterLivedDependencies` in the container options.

```js
const makePrintTime = ({ time }) => () => {
Expand Down Expand Up @@ -830,6 +839,10 @@ Args:
**_must_** be named exactly like they are in the registration. For
example, a dependency registered as `repository` cannot be referenced in a
class constructor as `repo`.
- `options.errorOnShorterLivedDependencies`: If `true`, will throw an error if
a singleton depends on a scoped or transient registration, or if a scoped
registration depends on a transient registration. Defaults to
`false`.

## `asFunction()`

Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

82 changes: 82 additions & 0 deletions src/__tests__/container.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ describe('container', () => {
expect(scope2.cradle.counterValue === 2).toBe(true)

expect(scope1Child.cradle.counterValue === 3).toBe(true)
// assert that the parent scope was not affected
expect(scope1.cradle.counterValue === 1).toBe(true)
})

it('supports nested scopes', () => {
Expand Down Expand Up @@ -438,6 +440,86 @@ describe('container', () => {
expect(err.message).toContain('lol')
})

it('allows longer lifetime modules to depend on shorter lifetime dependencies by default', () => {
const container = createContainer()
container.register({
first: asFunction((cradle: any) => cradle.second, {
lifetime: Lifetime.SCOPED,
}),
second: asFunction(() => 'hah'),
})

expect(container.resolve('first')).toBe('hah')
})

it('throws an AwilixResolutionError when longer lifetime modules depend on shorter lifetime dependencies and errorOnShorterLivedDependencies is set', () => {
const container = createContainer({
errorOnShorterLivedDependencies: true,
})
container.register({
first: asFunction((cradle: any) => cradle.second, {
lifetime: Lifetime.SCOPED,
}),
second: asFunction(() => 'hah'),
})

const err = throws(() => container.resolve('first'))
expect(err.message).toContain('first -> second')
expect(err.message).toContain(
"Dependency 'second' has a shorter lifetime than its parent: 'first'",
)
})

it('does not throw an error when an injector proxy is used and errorOnShorterLivedDependencies is set', () => {
const container = createContainer({
errorOnShorterLivedDependencies: true,
})
container.register({
first: asFunction((cradle: any) => cradle.injected, {
lifetime: Lifetime.SCOPED,
}).inject(() => ({ injected: 'hah' })),
})

expect(container.resolve('first')).toBe('hah')
})

it('allows for asValue() to be used when errorOnShorterLivedDependencies is set', () => {
const container = createContainer({
errorOnShorterLivedDependencies: true,
})
container.register({
first: asFunction((cradle: any) => cradle.val, {
lifetime: Lifetime.SCOPED,
}),
second: asFunction((cradle: any) => cradle.secondVal, {
lifetime: Lifetime.SINGLETON,
}),
val: asValue('hah'),
secondVal: asValue('foobar', { lifetime: Lifetime.SINGLETON }),
})

expect(container.resolve('first')).toBe('hah')
expect(container.resolve('second')).toBe('foobar')
})

it('correctly errors when a singleton parent depends on a scoped value and errorOnShorterLivedDependencies is set', () => {
const container = createContainer({
errorOnShorterLivedDependencies: true,
})
container.register({
first: asFunction((cradle: any) => cradle.second, {
lifetime: Lifetime.SINGLETON,
}),
second: asValue('hah'),
})

const err = throws(() => container.resolve('first'))
expect(err.message).toContain('first -> second')
expect(err.message).toContain(
"Dependency 'second' has a shorter lifetime than its parent: 'first'",
)
})

it('behaves properly when the cradle is returned from an async function', async () => {
const container = createContainer()
container.register({ value: asValue(42) })
Expand Down
15 changes: 15 additions & 0 deletions src/__tests__/lifetime.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Lifetime, isLifetimeLonger } from '../lifetime'

describe('isLifetimeLonger', () => {
it('correctly compares lifetimes', () => {
fnimick marked this conversation as resolved.
Show resolved Hide resolved
expect(isLifetimeLonger(Lifetime.TRANSIENT, Lifetime.TRANSIENT)).toBe(false)
expect(isLifetimeLonger(Lifetime.TRANSIENT, Lifetime.SCOPED)).toBe(false)
expect(isLifetimeLonger(Lifetime.TRANSIENT, Lifetime.SINGLETON)).toBe(false)
expect(isLifetimeLonger(Lifetime.SCOPED, Lifetime.TRANSIENT)).toBe(true)
expect(isLifetimeLonger(Lifetime.SCOPED, Lifetime.SCOPED)).toBe(false)
expect(isLifetimeLonger(Lifetime.SCOPED, Lifetime.SINGLETON)).toBe(false)
expect(isLifetimeLonger(Lifetime.SINGLETON, Lifetime.TRANSIENT)).toBe(true)
expect(isLifetimeLonger(Lifetime.SINGLETON, Lifetime.SCOPED)).toBe(true)
expect(isLifetimeLonger(Lifetime.SINGLETON, Lifetime.SINGLETON)).toBe(false)
})
})
39 changes: 31 additions & 8 deletions src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from './resolvers'
import { last, nameValueToObject, isClass } from './utils'
import { InjectionMode, InjectionModeType } from './injection-mode'
import { Lifetime } from './lifetime'
import { Lifetime, LifetimeType, isLifetimeLonger } from './lifetime'
import { AwilixResolutionError, AwilixTypeError } from './errors'
import { importModule } from './load-module-native.js'

Expand Down Expand Up @@ -187,6 +187,7 @@ export type ClassOrFunctionReturning<T> = FunctionReturning<T> | Constructor<T>
export interface ContainerOptions {
require?: (id: string) => any
injectionMode?: InjectionModeType
errorOnShorterLivedDependencies?: boolean
}

/**
Expand All @@ -197,7 +198,8 @@ export type RegistrationHash = Record<string | symbol | number, Resolver<any>>
/**
* Resolution stack.
*/
export interface ResolutionStack extends Array<string | symbol> {}
export interface ResolutionStack
extends Array<{ name: string | symbol; lifetime: LifetimeType }> {}

/**
* Family tree symbol.
Expand Down Expand Up @@ -451,7 +453,7 @@ export function createContainer<T extends object = any, U extends object = any>(
try {
// Grab the registration by name.
const resolver = getRegistration(name)
if (resolutionStack.indexOf(name) > -1) {
if (resolutionStack.some(({ name: parentName }) => parentName === name)) {
throw new AwilixResolutionError(
name,
resolutionStack,
Expand Down Expand Up @@ -497,13 +499,33 @@ export function createContainer<T extends object = any, U extends object = any>(
throw new AwilixResolutionError(name, resolutionStack)
}

// Pushes the currently-resolving module name onto the stack
resolutionStack.push(name)
const lifetime = resolver.lifetime || Lifetime.TRANSIENT

// if any of the parents have a shorter lifetime than the one requested,
// and the container is configured to do so, throw an error.
if (options?.errorOnShorterLivedDependencies) {
const maybeLongerLifetimeParentIndex = resolutionStack.findIndex(
({ lifetime: parentLifetime }) =>
isLifetimeLonger(parentLifetime, lifetime),
)
if (maybeLongerLifetimeParentIndex > -1) {
throw new AwilixResolutionError(
name,
resolutionStack,
`Dependency '${name.toString()}' has a shorter lifetime than its parent: '${resolutionStack[
fnimick marked this conversation as resolved.
Show resolved Hide resolved
maybeLongerLifetimeParentIndex
].name.toString()}'`,
)
}
}

// Pushes the currently-resolving module information onto the stack
resolutionStack.push({ name, lifetime })

// Do the thing
let cached: CacheEntry | undefined
let resolved
switch (resolver.lifetime || Lifetime.TRANSIENT) {
switch (lifetime) {
case Lifetime.TRANSIENT:
// Transient lifetime means resolve every time.
resolved = resolver.resolve(container)
Expand All @@ -521,8 +543,9 @@ export function createContainer<T extends object = any, U extends object = any>(
case Lifetime.SCOPED:
// Scoped lifetime means that the container
// that resolves the registration also caches it.
// When a registration is not found, we travel up
// the family tree until we find one that is cached.
// If this container cache does not have it,
fnimick marked this conversation as resolved.
Show resolved Hide resolved
// resolve and cache it rather than using the parent
// container's cache.
cached = container.cache.get(name)
if (cached !== undefined) {
// We found one!
Expand Down
14 changes: 5 additions & 9 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,11 @@ export class AwilixResolutionError extends AwilixError {
resolutionStack: ResolutionStack,
message?: string,
) {
if (typeof name === 'symbol') {
name = (name as any).toString()
}
resolutionStack = resolutionStack.map((val) =>
typeof val === 'symbol' ? (val as any).toString() : val,
)
resolutionStack.push(name)
const resolutionPathString = resolutionStack.join(' -> ')
let msg = `Could not resolve '${name as any}'.`
const stringName = name.toString()
const nameStack = resolutionStack.map(({ name: val }) => val.toString())
nameStack.push(stringName)
const resolutionPathString = nameStack.join(' -> ')
let msg = `Could not resolve '${stringName}'.`
if (message) {
msg += ` ${message}`
}
Expand Down
7 changes: 7 additions & 0 deletions src/lifetime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,10 @@ export const Lifetime: Record<LifetimeType, LifetimeType> = {
*/
SCOPED: 'SCOPED',
}

fnimick marked this conversation as resolved.
Show resolved Hide resolved
export function isLifetimeLonger(a: LifetimeType, b: LifetimeType): boolean {
return (
(a === Lifetime.SINGLETON && b !== Lifetime.SINGLETON) ||
(a === Lifetime.SCOPED && b === Lifetime.TRANSIENT)
)
}
7 changes: 6 additions & 1 deletion src/resolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,19 +116,24 @@ export type Constructor<T> = { new (...args: any[]): T }

/**
* Creates a simple value resolver where the given value will always be resolved.
* The lifetime of the resolved value defaults to `Lifetime.SCOPED`.
*
* @param {string} name
* The name to register the value as.
*
* @param {*} value
* The value to resolve.
*
* @param {object} opts
* Additional options for this resolver.
*
* @return {object}
* The resolver.
*/
export function asValue<T>(value: T): Resolver<T> {
export function asValue<T>(value: T, opts?: ResolverOptions<T>): Resolver<T> {
return {
resolve: () => value,
...{ lifetime: Lifetime.SCOPED, ...opts },
fnimick marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down