-
Notifications
You must be signed in to change notification settings - Fork 724
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 allowLazyInSync
container option
#1461
Conversation
12af285
to
c92b1ba
Compare
I think this is fine. @paul-marechal can you please make sure the CI get's to green ? |
c92b1ba
to
aef1b71
Compare
This is useful when binding promise objects while still using the synchronous APIs.
aef1b71
to
1e217fb
Compare
@@ -579,7 +588,7 @@ class Container implements interfaces.Container { | |||
): (T | T[]) { | |||
const result = this._get<T>(getArgs); | |||
|
|||
if (isPromiseOrContainsPromise<T>(result)) { | |||
if (!this.options.allowLazyInSync && isPromiseOrContainsPromise<T>(result)) { | |||
throw new Error(ERROR_MSGS.LAZY_IN_SYNC(getArgs.serviceIdentifier)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getArgs.serviceIdentifier
can be a symbol here, and it fails to get converted to string in LAZY_IN_SYNC
.
I added a quick fix in error_msgs.ts
to not fail in such a case. Not sure if the impl is good:
const TO_STRING = (value: any) => typeof value?.toString === 'function' ? value.toString() : `${value}`;
Surprisingly, doing the following fails:
const symbol = Symbol('test');
console.log(`${symbol}`);
But calling .toString()
manually on the symbol works... Gotta love working with JavaScript :)
@PodaruDragos CI should be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closing because I realized it might be weird to try and bend Inversify in way it isn't meant to be. I don't know what you guys think about the constraint with never binding promises/thenables but it sounds like it makes sense to encourage binding actual implementations and prevent raw promises from being passed around? |
This is useful when binding promise objects while still using the
synchronous APIs.
Description
Add a new
allowLazyInSync
option tointerfaces.ContainerOptions
.It is
false
by default.When
true
, it will prevent throwing when getting promise objects from the bindings.Related Issue
n/a
Motivation and Context
We used an old version of Inversify (5.1.1) and wanted to upgrade to 6.0.1.
We used to bind some promise objects as
.toConstantValue(promise...)
and have the components deal with it after construction. This now seem to completely break with the new Inversify logic.We don't have time to completely refactor our code to avoid the error from being thrown, so turning the check off is our best bet for now.
How Has This Been Tested?
Added a new unit test for this option.
Types of changes
Checklist: