-
Notifications
You must be signed in to change notification settings - Fork 6k
DynamicFeatureChannel MethodChannel and Install state tracking #22833
Conversation
| import org.json.JSONException; | ||
| import org.json.JSONObject; | ||
|
|
||
| public class SplitAotChannel { |
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.
Just coming in early to comment on the name. Can we s/SplitAot/DeferredFeature or DeferredModule here and elsewhere in docs and instructions etc? Since "split AOT" is a strict subset of what we're really shipping.
12de21d to
3497f87
Compare
ci/licenses_golden/licenses_flutter
Outdated
| FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformViewsChannel.java | ||
| FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java | ||
| FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/SettingsChannel.java | ||
| FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/SplitAotChannel.java |
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.
do you need to change this too?
ed1f09f to
1eef597
Compare
|
Framework paired PR: flutter/flutter#71879 |
0bfd131 to
dae9850
Compare
dae9850 to
b9dd22d
Compare
| textInputChannel = new TextInputChannel(dartExecutor); | ||
|
|
||
| if (dynamicFeatureManager != null) { | ||
| dynamicFeatureManager.setDynamicFeatureChannel(dynamicFeatureChannel); |
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.
if you create circular connections, best to break them yourself. Especially when there are singletons like JNI involved.
| * Gets the current state of the installation session corresponding to the specified loadingUnitId | ||
| * and/or moduleName. | ||
| * | ||
| * <p>Invocations of {@link installDynamicFeature} typically result in asynchronous downloading |
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.
though you can check the completion of that call by just waiting for that future itself no? Explain why they need this
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 is for displaying things like loading bars, status messages, etc. The status here is purely informational and is not necessary for function. I'll add that to the docs.
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.
I thought you added somewhere the redundancy with loadLibrary. Best be explicit about the web that exists everywhere. i.e. if you installDynamicFeature, you don't really have to check this or listen or the channel, if you listen on the channel, you don't really have to wait for the loadLibrary future, if you loadLibrary, it's the same as installDynamicFeature with loading unit etc, and fully create the web and cross reference everything to everything.
i.e. we don't want users guessing there's 3-4 APIs that can more or less do the same thing. What am I missing out if I just use 1 and don't use all of them.
| sessionIdToLoadingUnitId.get(sessionId), sessionIdToName.get(sessionId)); | ||
| } | ||
| if (channel != null) { | ||
| channel.completeInstallSuccess(sessionIdToName.get(sessionId)); |
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.
to make sure I understand, this is purely optional right? i.e. if a user calls loadLibrary in Dart (which ultimately leads here), they can just await that call instead of listening to this right?
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.
Yes, the methodChannel lives in parallel with the loadLibrary() code path. Since assets only modules do not have any library to load off of, this manual methodChannel way mirrors the functionality but allows specifying the moduleName directly. The channel accepts a loadingUnitId and a moduleName, but in the dart-side wrapper API I plan on exposing will restrict this to just taking a moduleName for now. The full loadingUnitId+moduleName API can be exposed in the future if/when dart allows initiating a loading unit load directly by loading unit id without a matching dart library loadLibrary() call.
shell/platform/android/io/flutter/embedding/engine/systemchannels/DynamicFeatureChannel.java
Outdated
Show resolved
Hide resolved
xster
left a comment
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
| flutterJNI.removeEngineLifecycleListener(engineLifecycleListener); | ||
| flutterJNI.setDynamicFeatureManager(null); | ||
| flutterJNI.detachFromNativeAndReleaseResources(); | ||
| dynamicFeatureChannel.destroy(); |
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.
maybe just do if feature manager is not null, set channel to null in the if below? So we don't have to create a new channel destroy pattern.
| * installed dynamic features. | ||
| * | ||
| * <p>Since this class may be instantiated for injection before the FlutterEngine and System | ||
| * Channels are initialized, this method should be called to provide the DynamicFeatureChannel. |
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.
We're also telling implementers to now take this and do stuff with it right? Describe what's expected to be sent when on this channel if you subclass.
| * Gets the current state of the installation session corresponding to the specified loadingUnitId | ||
| * and/or moduleName. | ||
| * | ||
| * <p>Invocations of {@link installDynamicFeature} typically result in asynchronous downloading |
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.
I thought you added somewhere the redundancy with loadLibrary. Best be explicit about the web that exists everywhere. i.e. if you installDynamicFeature, you don't really have to check this or listen or the channel, if you listen on the channel, you don't really have to wait for the loadLibrary future, if you loadLibrary, it's the same as installDynamicFeature with loading unit etc, and fully create the web and cross reference everything to everything.
i.e. we don't want users guessing there's 3-4 APIs that can more or less do the same thing. What am I missing out if I just use 1 and don't use all of them.
| String resolvedModuleName = | ||
| moduleName != null ? moduleName : loadingUnitIdToModuleName(loadingUnitId); | ||
| if (resolvedModuleName == null) { | ||
| Log.d(TAG, "Dynamic feature module name was null."); |
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 is probably a more prominent error right? If they look up something that don't exist, don't silently fail.
|
This pull request is not suitable for automatic merging in its current state.
|
Adds a
DynamicFeatureChannelfor manual invocation and tracking of the deferred library/asset split process.