Skip to content
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

fix FluentNavMenu.razor.js for non SSR #1560

Merged
merged 5 commits into from
Feb 26, 2024
Merged

fix FluentNavMenu.razor.js for non SSR #1560

merged 5 commits into from
Feb 26, 2024

Conversation

c0g1t8
Copy link
Contributor

@c0g1t8 c0g1t8 commented Feb 23, 2024

Pull Request

πŸ“– Description

Fix is related to #1387 and specifically #1387 (comment).

🎫 Issues

As stated in the comment, the collapse/expanding was being handled with the javacript added for SSR. The original code works correctly for all fluentblazor projects but not for fluentblazorwasm projects.

πŸ‘©β€πŸ’» Reviewer Notes

The fix provided is to prevent the addition of EventListeners when WASM rendered. It works with .net8 .

πŸ“‘ Test Plan

The fix has been tested for:

  • fluentblazorwasm
  • for all four fluentblazor interative rendering modes.

Note:

The EventListeners for collapse/expand disappear with interactive render modes Server, WebAssembly, and Auto. When stepping through the original FluentNavMenu.razor.js with the debugger, you can see the eventlisteners being attached. Then suddenly disappear. This may be related to the fact that the expander element also has a blazor based onClick handler.

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Feb 23, 2024

@dvoituron

Where does this command come from? Do you have a reference in the documentation?

I'm calling the same DotNet object that is used to Invoke a static .NET method. It comes from the _"framework/blazor.*.js" file.

I'm checking for the existence of a Symbol of 'wasm type'. This PR is based on using something undocumented and possibly implementation specific to .net8. Rejecting it on this basis is reasonable. πŸ™‚

What happens in "auto" mode where the component is initially displayed with interactive server-side rendering (interactive SSR) using the Blazor Server hosting model. The .NET WASM runtime and the entire application are downloaded to the client in the background and cached for use on subsequent visits.

I see your point. Is it interactive before the switch to the WASM runtime? I need to test that again.

Retagging PR as Draft

@c0g1t8 c0g1t8 changed the title fix FluentNavMenu.razor.js for non SSR [WIP] fix FluentNavMenu.razor.js for non SSR Feb 23, 2024
@c0g1t8 c0g1t8 marked this pull request as draft February 23, 2024 19:58
@c0g1t8 c0g1t8 changed the title [WIP] fix FluentNavMenu.razor.js for non SSR fix FluentNavMenu.razor.js for non SSR Feb 23, 2024
@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Feb 23, 2024

@dvoituron

I did some more testing of the PR using Auto interactivity. Interactivity is available before switching to WASM.

I created a new fluentblazor project using Auto interactivity. I then, removed the dependency on the nuget package and added a dependency to a local copy of Microsoft.FluentUI.AspNetCore.Components. Edited the FluentNavMenu.razor.js file and added a console log message to the toggleMenuExpandedAsync function. Set the browser to throttle network and ran the project.

I started clicking the expander toggle as soon as it appeared. The console log showed that the javacript function was being called. After a few clicks, the console log did not show the javascript function being called but the menu was being toggled.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Feb 24, 2024

@dvoituron

I found some documentation on the ASP.NET Core Blazor startup process. A Blazor object is exposed in javascript as part of the startup process. This mean isRunningInWASM can be rewritten with Blazor.runtime.

function isRunningInWASM() {
    return Object.getOwnPropertyNames(Blazor.runtime).length > 0
}

Blazor.runtime only has properties when running WASM.

During the startup process, the onLoad function in FluentNavMenu.razor.js is called. In all server rendered scenarios, Blazor.runtime has no properties since WASM hasn't started yet. The only time Blazor.runtime has properties during startup is when hosted client side in WASM.

@c0g1t8 c0g1t8 marked this pull request as ready for review February 24, 2024 04:54
@dvoituron
Copy link
Collaborator

I'm checking for the existence of a Symbol of 'wasm type'. This PR is based on using something undocumented and possibly implementation specific to .net8. Rejecting it on this basis is reasonable. πŸ™‚

It's actually this part that disturbs me a little: using an element that is not documented and can therefore change at any time is very risky in my opinion. I don't think it's a good idea. It would be better to find another "safe" way of detecting whether you're in WASM or not. One solution might be to provide an isRunningInWASM property which returns this value, but which could be overridden by the developer in the event of a problem (or a change in the ASPNET team).

What's more, does this remain valid in all other modes of use, such as MAUI for example?

@vnbaaij
Copy link
Collaborator

vnbaaij commented Feb 26, 2024

@dvoituron

I found some documentation on the ASP.NET Core Blazor startup process. A Blazor object is exposed in javascript as part of the startup process. This mean isRunningInWASM can be rewritten with Blazor.runtime.

function isRunningInWASM() {
    return Object.getOwnPropertyNames(Blazor.runtime).length > 0
}

Blazor.runtime only has properties when running WASM.

During the startup process, the onLoad function in FluentNavMenu.razor.js is called. In all server rendered scenarios, Blazor.runtime has no properties since WASM hasn't started yet. The only time Blazor.runtime has properties during startup is when hosted client side in WASM.

This is the way to go indeed. The Blazor object is not going to change anytime soon (if at all). But interested to see if this also works with Blazor Hybrid/MAUI setup as @dvoituron asked.

@vnbaaij vnbaaij self-requested a review February 26, 2024 16:41
@vnbaaij vnbaaij enabled auto-merge (squash) February 26, 2024 16:41
@vnbaaij vnbaaij merged commit 8ecf0f2 into microsoft:dev Feb 26, 2024
4 checks passed
@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Feb 26, 2024

I didn't test the Blazor Hybrid/MAUI scenarios. I'll try to run a test before the end of the week and respond here.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Feb 27, 2024

Thanks Gary!

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 2, 2024

@vnbaaij

I finally got around to running a Blazor Hybrid/MAUI test. This fix does not cover this scenario. The javascript listeners are still being attached.

This makes sense given that MAUI generates native applications and WASM is not needed. A test to see if the component is running in Blazor Hybrid/MAUI is needed.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 4, 2024

@vnbaaij

I think I have a fix for this. I'll create a draft PR later today. πŸ™‚

@c0g1t8 c0g1t8 deleted the fixSSR branch March 12, 2024 18:58
c0g1t8 added a commit to c0g1t8/fluentui-blazor that referenced this pull request Mar 17, 2024
c0g1t8 added a commit to c0g1t8/fluentui-blazor that referenced this pull request Mar 17, 2024
vnbaaij added a commit that referenced this pull request Mar 18, 2024
…lone (#1633)

* prevents listeners from being added when running in BlazorWebView

* Revert "prevents listeners from being added when running in BlazorWebView"

This reverts commit 772b1e0.

* Revert "fix FluentNavMenu.razor.js for non SSR (#1560)"

This reverts commit 8ecf0f2.

* fix - only define custom element fluent-page-script for Blazor Web Apps

---------

Co-authored-by: Vincent Baaij <vnbaaij@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants