Skip to content

Commit 917322c

Browse files
committed
handle errors from instance factory
1 parent 059fb5d commit 917322c

File tree

2 files changed

+71
-14
lines changed

2 files changed

+71
-14
lines changed

packages/component/src/provider.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,40 @@ describe('Provider', () => {
4949
).to.not.throw();
5050
});
5151

52+
it('does not throw if instance factory throws when calling getImmediate() with optional flag', () => {
53+
provider.setComponent(getFakeComponent('test', () => { throw Error('something went wrong!'); }));
54+
expect(() => provider.getImmediate({ optional: true })).to.not.throw();
55+
});
56+
57+
it('throws if instance factory throws when calling getImmediate() without optional flag', () => {
58+
provider.setComponent(getFakeComponent('test', () => { throw Error('something went wrong!'); }));
59+
expect(() => provider.getImmediate()).to.throw();
60+
});
61+
62+
it('does not throw if instance factory throws when calling get()', () => {
63+
provider.setComponent(getFakeComponent('test', () => { throw Error('something went wrong!'); }));
64+
expect(() => provider.get()).to.not.throw();
65+
});
66+
67+
it('does not throw if instance factory throws when registering an eager component', () => {
68+
const eagerComponent = getFakeComponent(
69+
'test',
70+
() => { throw Error('something went wrong!'); },
71+
false,
72+
InstantiationMode.EAGER
73+
);
74+
75+
expect(() => provider.setComponent(eagerComponent)).to.not.throw();
76+
});
77+
78+
it('does not throw if instance factory throws when registering a component with a pending promise', () => {
79+
// create a pending promise
80+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
81+
provider.get();
82+
const component = getFakeComponent('test', () => { throw Error('something went wrong!'); });
83+
expect(() => provider.setComponent(component)).to.not.throw();
84+
});
85+
5286
describe('Provider (multipleInstances = false)', () => {
5387
describe('getImmediate()', () => {
5488
it('throws if the service is not available', () => {

packages/component/src/provider.ts

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export class Provider<T extends Name> {
3636
constructor(
3737
private readonly name: T,
3838
private readonly container: ComponentContainer
39-
) {}
39+
) { }
4040

4141
/**
4242
* @param identifier A provider can provide mulitple instances of a service
@@ -50,9 +50,14 @@ export class Provider<T extends Name> {
5050
const deferred = new Deferred<NameServiceMapping[T]>();
5151
this.instancesDeferred.set(normalizedIdentifier, deferred);
5252
// If the service instance is available, resolve the promise with it immediately
53-
const instance = this.getOrInitializeService(normalizedIdentifier);
54-
if (instance) {
55-
deferred.resolve(instance);
53+
try {
54+
const instance = this.getOrInitializeService(normalizedIdentifier);
55+
if (instance) {
56+
deferred.resolve(instance);
57+
}
58+
} catch (e) {
59+
// when the instance factory throws an exception during get(), it should not cause
60+
// an fatal error. We just return the unresolved promise in this case.
5661
}
5762
}
5863

@@ -86,17 +91,24 @@ export class Provider<T extends Name> {
8691
};
8792
// if multipleInstances is not supported, use the default name
8893
const normalizedIdentifier = this.normalizeInstanceIdentifier(identifier);
94+
try {
95+
const instance = this.getOrInitializeService(normalizedIdentifier);
8996

90-
const instance = this.getOrInitializeService(normalizedIdentifier);
97+
if (!instance) {
98+
if (optional) {
99+
return null;
100+
}
101+
throw Error(`Service ${this.name} is not available`);
102+
}
91103

92-
if (!instance) {
104+
return instance;
105+
} catch (e) {
93106
if (optional) {
94107
return null;
108+
} else {
109+
throw e;
95110
}
96-
throw Error(`Service ${this.name} is not available`);
97111
}
98-
99-
return instance;
100112
}
101113

102114
setComponent(component: Component<T>): void {
@@ -113,7 +125,14 @@ export class Provider<T extends Name> {
113125
this.component = component;
114126
// if the service is eager, initialize the default instance
115127
if (isComponentEager(component)) {
116-
this.getOrInitializeService(DEFAULT_ENTRY_NAME);
128+
try {
129+
this.getOrInitializeService(DEFAULT_ENTRY_NAME);
130+
} catch (e) {
131+
// when the instance factory for an eager Component throws an exception during the eager
132+
// initialization, it should not cause an fatal error.
133+
// TODO: Investigate if we need to make it configurable, because some component may want to cause
134+
// a fatal error in this case?
135+
}
117136
}
118137

119138
// Create service instances for the pending promises and resolve them
@@ -127,10 +146,14 @@ export class Provider<T extends Name> {
127146
instanceIdentifier
128147
);
129148

130-
// `getOrInitializeService()` should always return a valid instance since a component is guaranteed. use ! to make typescript happy.
131-
const instance = this.getOrInitializeService(normalizedIdentifier)!;
132-
133-
instanceDeferred.resolve(instance);
149+
try {
150+
// `getOrInitializeService()` should always return a valid instance since a component is guaranteed. use ! to make typescript happy.
151+
const instance = this.getOrInitializeService(normalizedIdentifier)!;
152+
instanceDeferred.resolve(instance);
153+
} catch (e) {
154+
// when the instance factory throws an exception, it should not cause
155+
// an fatal error. We just leave the promise unresolved.
156+
}
134157
}
135158
}
136159

0 commit comments

Comments
 (0)