Fixes #146 #149
Fixes #146 #149
Conversation
|
CI is failing because of |
lib/src/google_fonts_base.dart
Outdated
| if (fontData == null) return; | ||
|
|
||
| final fontLoader = FontLoader(familyWithVariantString); | ||
| fontLoader.addFont(byteData as Future<ByteData>); |
There was a problem hiding this comment.
Should it be like this?
fontLoader.addFont(Future.value(fontData));
There was a problem hiding this comment.
I don't think so. What does this get us?
There was a problem hiding this comment.
Because addFont requires a Future<ByteData> as input, and the value is already loaded, we can simply pack it inside a Future.value and pass it in.
There was a problem hiding this comment.
Could it take FutureOr<ByteData>? Avoiding an unnecessary wrap would be nice.
There was a problem hiding this comment.
Also, it seems this case isn't covered in tests, right?
There was a problem hiding this comment.
It takes only Future<ByteData>
There was a problem hiding this comment.
The current impl just reuses the existing future, right? That should be fine – instead of creating a new one.
There was a problem hiding this comment.
Exactly, the current uses the existing future.
There was a problem hiding this comment.
@gaetschwartz I think you'll need to wrap the value. So you have a Future<ByteData> instead of Future<ByteData?>
Have you tested this workflow to make sure it works? Are there tests?
There was a problem hiding this comment.
I ran the examples and it worked without but I can still do if you want
|
@gaetschwartz – we have zero tests here, it appears. have you verified that your latest bits pass? Should we add tests? Seems like a gnarly thing to let through w/out verification |
|
Alright, can I make the method public with @vsisibleForTesting ? @kevmoo |
Sure! |
|
@kevmoo After re-reading the code I don't really think we need to make a specific test as the only place where it is used is TLDR; I don't think we need tests, thoughts ? |
|
I defer to the package owners here, I guess. Just don't want this blowing up in production! |
|
Understandably. Is it @pennzht ? |
I think we should add a test if it fails the existing version (= if it can replicate and catch the bug #146). |
|
@pennzht I don't really know how to mock |
|
|
||
| test('loadFontByteData doesn\'t fail', () { | ||
| expect( | ||
| () async => loadFontByteData('fontFamily', Future.value(ByteData(0))), | ||
| returnsNormally, | ||
| ); | ||
| expect( | ||
| () async => loadFontByteData('fontFamily', Future.value(null)), | ||
| returnsNormally, | ||
| ); | ||
| expect( | ||
| () async => loadFontByteData('fontFamily', null), | ||
| returnsNormally, | ||
| ); | ||
|
|
||
| expect( | ||
| () async => loadFontByteData('fontFamily', | ||
| Future.delayed(Duration(milliseconds: 100), () => null)), | ||
| returnsNormally, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I added this test, thoughts ?
|
@clocksmith – would you merge? |
|
@gaetschwartz Thank you so much for your PR! |
|
@pennzht – are you going to merge? |
This PR fixes #146.
As per discussed in #148 it would be better to tackle this issue first, so here is the PR.
WWhile running the tests, the
google-fonts-flutter\test\load_font_if_necessary_test.dart: loadFontIfNecessary method calls http gettest hangs indefinitely regardless of my fix or not. Am I doing anything wrong ?