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

Custom HTML elements rendering Blazor components #42314

Merged
merged 8 commits into from
Jun 23, 2022

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jun 20, 2022

Custom HTML elements rendering Blazor components

This PR productizes the custom elements support package.

Description

The productized version of the support package is functionally equivalent to the original. For the next preview release, we may want to consider addressing #38369 (comment).

Validation:

  • E2E tests
  • Tested manually

Fixes #38447
Fixes #42329

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner June 20, 2022 23:50
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jun 20, 2022
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll let @SteveSandersonMS have the final say.

@SteveSandersonMS
Copy link
Member

Ah, this is interesting! I didn't realize we were going to merge the code into the core framework. I had expected "productize" only to mean "turn this experimental package into a supported package" (so it would still be a separate package, but now it's official and supported and built from the main repo).

@MackinnonBuck Sorry that we weren't clear about this before. We really should not write issues without any meaningful description. Do you think there's any substantial technical requirement to merge the code into the core framework?

My personal preference would be to err on the side of having this as an optional package because:

  • Avoids adding weight to blazor.*.js for people who aren't using it. I know it's not a huge amount in this one case but as time goes on and we add more features, we increasingly need to avoid expanding the core by default.
    • ... and if we want to add a bunch more custom-elements-specific JS functionality to this code in the future, we can be much freer to do so if it's not going to affect non-users
  • There isn't necessarily only one true way to do custom elements. It would be reasonable for people to use a different package if they have a different approach to custom elements.
  • It gives us more flexibility in the future: we can always non-breakingly push functionality down into the core, but we can never lift it out into an external optional package.
  • Keeping things out of core where possible forces us to architect things with clean boundaries so complex things can be achieved in user code, don't rely on internal APIs, and keeps the core more practical to reason about

The one counterpoint would be that having it as an external package means developers have to do a gesture to add that extra package before they can use it. Personally I don't think this extra friction outweighs the other considerations.

@MackinnonBuck What do you think?

@javiercn
Copy link
Member

I agree with @SteveSandersonMS.

I thought it was going to be a separate package. If there is JS we need to deliver to initialize Blazor, we can do so via a JS initializer, so the gesture remains "add a package" and everything should work from there.

@MackinnonBuck
Copy link
Member Author

Ah, thanks for the clarification, @SteveSandersonMS! Sorry for the misunderstanding - I agree with your preference of having this as an optional package.

@MackinnonBuck
Copy link
Member Author

I'll update this PR, but this may need to get pushed to preview7 since code-complete is today

@javiercn
Copy link
Member

@MackinnonBuck if you put a decent PR out, @SteveSandersonMS and me can look at it tomorrow and merge it if we think it is good to go in the morning (our time).

@MackinnonBuck
Copy link
Member Author

Thanks @javiercn. I've just moved the custom elements feature to its own package Microsoft.AspNetCore.Components.CustomElements, so this should be ready for review again.

Copy link
Member

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Signing off on behalf of @javiercn!

@MackinnonBuck MackinnonBuck merged commit 8942608 into main Jun 23, 2022
@MackinnonBuck MackinnonBuck deleted the mbuck/custom-elements branch June 23, 2022 18:49
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
6 participants