Skip to content

[null safety] Bugfix in isRegistered() #147

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[null safety] Bugfix in isRegistered() #147

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 7 additions & 8 deletions lib/get_it_impl.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,12 +294,13 @@ class _GetItImplementation implements GetIt {
}
scopeLevel--;
}
assert(
instanceFactory != null,
'Object/factory with ${instanceName != null ? 'with name $instanceName and ' : ''}'
' type ${T.toString()} is not registered inside GetIt. '
'\n(Did you accidentally do GetIt sl=GetIt.instance(); instead of GetIt sl=GetIt.instance;'
'\nDid you forget to register it?)');
throwIf(
instanceFactory == null,
StateError(
'Object/factory with ${instanceName != null ? 'with name $instanceName and ' : ''}'
'type ${T.toString()} is not registered inside GetIt. '
'\n(Did you accidentally do GetIt sl=GetIt.instance(); instead of GetIt sl=GetIt.instance;'
'\nDid you forget to register it?)'));

return instanceFactory;
}
Expand Down Expand Up @@ -725,8 +726,6 @@ class _GetItImplementation implements GetIt {

for (final type in dependsOn) {
final dependentFactory = _findFactoryByNameAndType(null, type);
throwIf(dependentFactory == null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ArgumentError('Dependent Type $type is not registered in GetIt'));
throwIfNot(dependentFactory.canBeWaitedFor,
ArgumentError('Dependent Type $type is not an async Singleton'));
dependentFactory.objectsWaiting.add(serviceFactory.registrationType);
Expand Down