-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Follow-ups to lazy-load from API review #24169
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -324,9 +324,18 @@ function createEmscriptenModuleInstance(resourceLoader: WebAssemblyResourceLoade | |
const assembliesToLoad = BINDING.mono_array_to_js_array<System_String, string>(assembliesToLoadDotNetArray); | ||
const lazyAssemblies = resourceLoader.bootConfig.resources.lazyAssembly; | ||
|
||
if (lazyAssemblies) { | ||
const resourcePromises = Promise.all(assembliesToLoad | ||
.filter(assembly => lazyAssemblies.hasOwnProperty(assembly)) | ||
if (!lazyAssemblies) { | ||
throw new Error("No assemblies have been marked as lazy-loadable. Use the 'BlazorWebAssemblyLazyLoad' item group in your project file to enable lazy loading an assembly."); | ||
} | ||
|
||
var assembliesMarkedAsLazy = assembliesToLoad.filter(assembly => lazyAssemblies.hasOwnProperty(assembly)); | ||
|
||
if (assembliesMarkedAsLazy.length != assembliesToLoad.length) { | ||
var notMarked = assembliesToLoad.filter(assembly => !assembliesMarkedAsLazy.includes(assembly)); | ||
throw new Error(`${notMarked.join()} must be marked with 'BlazorWebAssemblyLazyLoad' item group in your project file to allow lazy-loading.`); | ||
} | ||
|
||
const resourcePromises = Promise.all(assembliesMarkedAsLazy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be returned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't see how this can work asynchronously without doing so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We return a promise in L305. This is the same thing we were doing before for this scenario. Do you mean that we need to return a Promise in the scenarios where we are currently throwing an error? This is the main change in this commit since we previously returned a dummy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, yes I think you're right @captainsafia. There was something about the diff (the removal of line 348) that made it look as if we were now discarding this promise. I didn't read it clearly enough. |
||
.map(assembly => resourceLoader.loadResource(assembly, `_framework/${assembly}`, lazyAssemblies[assembly], 'assembly')) | ||
.map(async resource => (await resource.response).arrayBuffer())); | ||
|
||
|
@@ -345,8 +354,6 @@ function createEmscriptenModuleInstance(resourceLoader: WebAssemblyResourceLoade | |
return resourcesToLoad.length; | ||
})); | ||
} | ||
return BINDING.js_to_mono_obj(Promise.resolve(0)); | ||
} | ||
}); | ||
|
||
module.postRun.push(() => { | ||
|
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.
Noice