Skip to content

Defer link interception until Router is initialized #10062

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 7 commits into from
May 17, 2019
Merged

Defer link interception until Router is initialized #10062

merged 7 commits into from
May 17, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented May 8, 2019

Fixes #9834

@pranavkm pranavkm requested a review from SteveSandersonMS as a code owner May 8, 2019 00:25
@pranavkm pranavkm requested a review from javiercn May 8, 2019 00:25
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 8, 2019
@pranavkm pranavkm force-pushed the prkrishn/link-interception branch 3 times, most recently from 27b891c to 454bab0 Compare May 8, 2019 21:20
@pranavkm
Copy link
Contributor Author

pranavkm commented May 8, 2019

🆙 📅

@SteveSandersonMS
Copy link
Member

Thanks for the update! Sorry I didn't get to review this today, but will make a priority of doing so tomorrow.

SteveSandersonMS
SteveSandersonMS reviewed May 10, 2019
View reviewed changes
src/Components/Blazor/Blazor/src/Services/WebAssemblyUriHelper.cs
@@ -28,7 +28,7 @@ internal WebAssemblyUriHelper()
protected override void EnsureInitialized()
{
WebAssemblyJSRuntime.Instance.Invoke<object>(
Interop.EnableNavigationInterception,
Interop.ListenForNavigationEvents,
Copy link
Member

Choose a reason for hiding this comment

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

Conceptually, this is so much clearer! 👍

@SteveSandersonMS
Copy link
Member

This looks really great!

One other E2E test that would be excellent to have would be showing that a component can observe the URL via IUriHelper even when there's no Router. Ideally it would also do JS interop to change the URL via pushState and see that it can still see the updated URL on the .NET side.

@pranavkm pranavkm force-pushed the prkrishn/link-interception branch 2 times, most recently from 91e7b5d to 87c0deb Compare May 16, 2019 16:52
@pranavkm
Copy link
Contributor Author

🈹 📅

SteveSandersonMS
SteveSandersonMS reviewed May 17, 2019
View reviewed changes
src/Components/test/testassets/TestServer/Startup.cs Outdated
@@ -59,6 +60,7 @@ public void Configure(IApplicationBuilder app, IWebHostEnvironment env)

subdirApp.UseEndpoints(endpoints =>
{
endpoints.Map("NotAComponent.html", context => context.Response.WriteAsync("<div id=\"test-info\">Not a component!</div>"));
Copy link
Member

Choose a reason for hiding this comment

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

Is this line still needed, consider that you added src/Components/test/testassets/BasicTestApp/wwwroot/NotAComponent.html?

SteveSandersonMS
SteveSandersonMS approved these changes May 17, 2019
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.

This is great, @pranavkm! Thanks for the updates. It all looks perfect now. I added one further comment about whether a certain line of code is strictly needed - please deal with or ignore this however you wish.

I think the client side router's behavior will make a lot more sense to people now. Allowing links to non-client-side URLs independently of <base href> concerns is extremely valuable.

@pranavkm pranavkm force-pushed the prkrishn/link-interception branch from fc6fbd9 to 4fb3819 Compare May 17, 2019 21:02
@pranavkm pranavkm merged commit b9546df into master May 17, 2019
@ghost ghost deleted the prkrishn/link-interception branch May 17, 2019 22:33
@SteveSandersonMS SteveSandersonMS mentioned this pull request Jun 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@javiercn javiercn Awaiting requested review from javiercn

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Blazor Component Embedded in Razor Page
3 participants
@pranavkm @SteveSandersonMS @Eilon