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

Blazor Web project interactive options improvements. Rename render modes. #50684

Merged
merged 23 commits into from
Sep 15, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Sep 13, 2023

Fixes #50433 (Add root level interactivity option)
Fixes #50646 (Remove workaround for Counter component)
Fixes #50636 (Clarify the names of the interactive render modes)

In terms of the code we now emit, there should be nothing controversial here. The template just has to do quite a bit of if/else in many places to account for all these options and how rendermodes are used and not used based on them.

The PR is big because the renames have really wide impact, but almost all the "files changes" are just due to renames. The only real code changes are in the project templates.

Testing impact

Adding this option, the BlazorWeb template now has so many possible combinations of options, including:

  • Whether or not to enable Server interactivity
  • Whether or not to enable WebAssembly interactivity
  • Whether or not to be interactive from the root
  • Whether or not to include sample content
  • Whether or not to use ProgramMain

So that's around 32 combinations of output - without even accounting for auth options! We don't currently have E2E verification of any of them, as those tests are skipped due to unreliability. We're going to have to lean hard on CTI validations for this, and make sure all the important combinations are covered - cc @mkArtakMSFT.

Options design update

Having a list of 6 separate checkboxes in VS is pretty unpleasant and hard to understand:

So, in this PR I'm proposing (and have implemented, but we can still change it), a change to use dropdowns for the interactivity type and location options. This reduces the number of inputs by one, and means they can be more self-describing:

  • The "interactivity type" choices are:
    • None
    • Server (default)
    • WebAssembly
    • Auto (Server and WebAssembly).
  • The "interactivity location" choices are:
    • Per page/component (default)
    • Global

Note that "interactivity location" is disabled if interactivity type == "None", but only CLI honors that right now (VS should add support later, and until then, location will have no effect if there's no interactivity).

I think this is much easier to understand, since you no longer have to infer that enabling both Server and WebAssembly means you're going to get Auto. It's also much better in the CLI, since it was completely ridiculous before that --use-server defaulted to true but --use-wasm defaulted to false, so to get WebAssembly you needed to set --use-server false --use wasm. Now you would say --interactivity webassembly (and not wasm - that was weird too).

image

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 13, 2023
@SteveSandersonMS SteveSandersonMS changed the title Blazor Web project globally-interactive option Blazor Web project interactive options improvements, and rename the render modes Sep 14, 2023
@SteveSandersonMS SteveSandersonMS changed the title Blazor Web project interactive options improvements, and rename the render modes Blazor Web project interactive options improvements. Rename render modes. Sep 14, 2023
@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Sep 14, 2023
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review September 14, 2023 11:24
Copy link
Member

@danroth27 danroth27 left a comment

Choose a reason for hiding this comment

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

Love the new UX! It's way better than the flat list of check boxes.

@@ -660,8 +748,8 @@
"AuthOption": "None"
},
"EmptyUseWebAssembly": {
"Template": "web",
"Arguments": "new blazor --use-server false --use-wasm --empty",
"Template": "blazor",
Copy link
Member

Choose a reason for hiding this comment

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

So this was a bug we didn't notice in the past then?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Sep 15, 2023

Choose a reason for hiding this comment

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

It was a weird inconsistency but didn't impact the test actually working correctly. I think the Template string is only used for reporting in logs, and doesn't affect whether the test passes or fails, or which template gets executed.

@davidfowl
Copy link
Member

These names don't look very nice... I don't have an alternative yet but these feel very verbose and not that much more clear.

@SteveSandersonMS
Copy link
Member Author

These names don't look very nice... I don't have an alternative yet but these feel very verbose and not that much more clear.

Since you mention verbosity, I'm guessing you're referring to the CLI names (inside the VS wizard, verbosity is not much of a consideration).

So, regarding CLI names, Dan and I discussed:

  • For "interactive at root" on the CLI (--all-interactive), we tried out various ideas but didn't find anything that seemed both clearer and more succinct. The most literal equivalent to the VS phrasing would be --interactivity-location global but that's way too long. If anyone comes up with an improved name we're happy to change to it.
  • For "interactivity type" on the CLI, we renamed from --interactivity to --interactive, so the usage will be --interactive webassembly or --interactive auto or --interactive none or --interactive server (the latter being default). We think this feels pretty good, and uses exactly the terminology developers and our docs all use already.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/project-interactive-at-root branch from a789c25 to 66e26a8 Compare September 15, 2023 10:17
@SteveSandersonMS
Copy link
Member Author

Update: we can't use the parameter name --interactive because that's a reserved alias. I've set it back to --interactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants