[null safety] Bugfix in isRegistered() #147
Conversation
| @@ -725,8 +726,6 @@ class _GetItImplementation implements GetIt { | |||
|
|
|||
| for (final type in dependsOn) { | |||
| final dependentFactory = _findFactoryByNameAndType(null, type); | |||
| throwIf(dependentFactory == null, | |||
There was a problem hiding this comment.
Hmm, this is now interesting. Hadn't thought of this consequences because clearly the Exception thrown here conatins more information that the more generic one that gets thrown be _findFactoryByNameAndType.
So either we would have to wrap the _findFactory call in a try-catch or switch back to your solution returning null.
I fear I have to revisit where finding errors should be handled, Because even in debug the assert in _findFactory had robbed you off that information.
There was a problem hiding this comment.
My way of writing code is not to meddle with try-catch too much and to prefer code, like I did before. What do you prefer?
There was a problem hiding this comment.
The reason why I put the asserts inside the _findFactory was that I wanted to avaoid having to put an error check everywhere it was used.
I will take the last version of your previous PR and compare it to the current source. As it's again formatted with 80 characters It should be easier to read.
As I never used isRegistered in my own Apps it was important that you found the bug.
This fixes a bug where production deployments will cause
isRegistered<T>()to crash whenTis not registered yet.