Skip to content

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.

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

Jump to bottom
Merged
merged 2 commits into from
Jul 22, 2020
Merged

Follow-ups to lazy-load from API review #24169

merged 2 commits into from
Jul 22, 2020

Conversation

@captainsafia
Copy link
Member

@captainsafia captainsafia commented Jul 21, 2020 •
edited
Loading

  • Make new parameters on Router nullable
  • Move check for OnNavigateAsync == null below await previousOnNavigate to ensure that previous on navigates complete even if onNavigateAsync parameter is removed
  • Use ExceptionDispatchInfo.Throw instead of ExceptionDispatchInfo.Capture.Throw for re-throwing errors and preserving context
  • Throw JSException from getLazyAssemblies on-call for invalid inputs
  • Change constructor for LazyAssemblyLoader to take injected IJSRuntime
  • Use Assembly.Load to verify if assemblies have already been loaded during pre-rendering
  • Add tests for catching exceptions thrown for invalid inputs

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 21, 2020
@captainsafia captainsafia changed the base branch from master to release/5.0-preview8 July 21, 2020 19:36
@captainsafia captainsafia force-pushed the safia/ll-api-review branch 2 times, most recently from 401110f to bff6766 Compare July 22, 2020 00:47
@captainsafia captainsafia marked this pull request as ready for review July 22, 2020 00:48
@captainsafia captainsafia requested review from a team and SteveSandersonMS as code owners July 22, 2020 00:48
pranavkm
pranavkm approved these changes Jul 22, 2020
View reviewed changes
src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts
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.");
Copy link
Contributor

@pranavkm pranavkm Jul 22, 2020

Choose a reason for hiding this comment

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

Noice

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts
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
Copy link
Contributor

@pranavkm pranavkm Jul 22, 2020

Choose a reason for hiding this comment

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

Does this need to be returned?

Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 22, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@captainsafia captainsafia Jul 22, 2020

Choose a reason for hiding this comment

The 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 Promise.resolve(0) if the parameter was invalid.

Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 22, 2020

Choose a reason for hiding this comment

The 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.

SteveSandersonMS
SteveSandersonMS approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Great. This is now a super thought-through feature :)

@captainsafia captainsafia force-pushed the safia/ll-api-review branch 3 times, most recently from e996f31 to d2d2af8 Compare July 22, 2020 19:53
@captainsafia captainsafia force-pushed the safia/ll-api-review branch from d2d2af8 to f98d0a4 Compare July 22, 2020 20:34
@captainsafia
Copy link
Member Author

captainsafia commented Jul 22, 2020

@mkArtakMSFT This should be good to merge now!

@mkArtakMSFT mkArtakMSFT merged commit 4734f47 into release/5.0-preview8 Jul 22, 2020
@mkArtakMSFT mkArtakMSFT deleted the safia/ll-api-review branch July 22, 2020 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

+1 more reviewer

@pranavkm pranavkm pranavkm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-blazor Includes: Blazor, Razor Components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@captainsafia @halter73 @pranavkm @SteveSandersonMS @mkArtakMSFT