-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
[flutter_tools] remove most globals from asset system and remove Cache manipulation in unit tests #70011
[flutter_tools] remove most globals from asset system and remove Cache manipulation in unit tests #70011
Conversation
@@ -48,12 +47,6 @@ $fontsSection | |||
'''); | |||
} | |||
|
|||
void establishFlutterRoot() { |
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.
All of this removed code is a no-op now that the material manifest is stored in code
@@ -389,11 +399,284 @@ class ManifestAssetBundle implements AssetBundle { | |||
)) font.descriptor, | |||
]; | |||
} | |||
|
|||
DevFSStringContent _createAssetManifest(Map<_Asset, List<_Asset>> assetVariants) { |
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.
This change mostly takes the top-level private methods and makes them methods on the ManifestAssetBundle instead.
The other change is moving file creation out of the _Asset class to avoid the direct dep on the filesystem
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.
this code could be simplified further but I chose to keep it mostly as is
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 with nits
} | ||
|
||
bool get assetFileExists => assetFile.existsSync(); | ||
// bool get assetFileExists => assetFile.existsSync(); |
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.
Remove
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.
Done
@@ -406,11 +689,11 @@ class _Asset { | |||
/// A platform-independent URL representing the entry for the asset manifest. | |||
final Uri entryUri; | |||
|
|||
File get assetFile { | |||
return globals.fs.file(globals.fs.path.join(baseDir, globals.fs.path.fromUri(relativeUri))); | |||
File createAssetFile(FileSystem fileSystem) { |
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.
This name sounds like it's creating the file.
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.
changed to lookupAssetFile
which hopefully sounds more like what it does
Description
Removes most of the easy globals - there is still indirect usage via the flutter project. Removes most of the special flutter root setup the asset bundle tests have.
#47161