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

Consider non-ASCII characters in HTML case regex #10866

Closed
wants to merge 3 commits into from

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 10, 2024

Fixes #10844.

I've replaced [a-z] with \p{Ll} (any lowercase letter) and [A-Z] with \p{Lu} (any uppercase letter).

Note that this is technically a breaking change, I'm not sure how we feel about that.

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Sep 10, 2024
@jjonescz jjonescz marked this pull request as ready for review September 10, 2024 13:33
@jjonescz jjonescz requested a review from a team as a code owner September 10, 2024 13:33
@333fred
Copy link
Member

333fred commented Sep 10, 2024

Note that this is technically a breaking change

What is something that would break with this change?

@davidwengier
Copy link
Contributor

From the issue, users with a component named TestÄh currently need to use a tag with <vc:testäa>, but after the change they would need to use <vc:test-äa>. Until they make the change in their source, they'd lose the functionality of the view component and the tag would simply be rendered to Html and presumably ignored by the browser.

@jjonescz
Copy link
Member Author

jjonescz commented Sep 11, 2024

As David says. Plus this also affects normal TagHelpers (not just ViewComponent tag helpers) - if users don't specify the HtmlTargetElement explicitly, it's derived from the TagHelper class name in the same fashion. (I will add tests.)

@333fred
Copy link
Member

333fred commented Sep 11, 2024

@chsienki, @jaredpar, @DustinCampbell, @danroth27 for their thoughts. I'm inclined to won't fix this bug; it's been around for a very long time, and we don't have a good way of communicating the change to users in the form of compilation errors. It's also another case where runtime compilation would probably produce different results, and there'd be no way to get equivalent behavior between runtime and non-runtime compilation.

@jjonescz
Copy link
Member Author

We could also tie the breaking change to RazorLangVersion. But I agree it does not seem worth it.

@danroth27
Copy link
Member

I'm supportive of not making this change given the breaking change impact.

@jjonescz jjonescz closed this Sep 12, 2024
@jjonescz jjonescz deleted the 10844-UnicodeVc branch September 12, 2024 08:53
@DavidZidar
Copy link

Hi, wouldn't it be possible to generate tag helpers with both the correct name and the current incorrect name? This would correct the bug while preserving backwards compatibility for those that rely on the current tag names.

@jjonescz
Copy link
Member Author

Yes, I think that would work, but at the cost of more complexity in the implementation.

@DavidZidar
Copy link

The problem my team is currently having is that the generated tag helpers for view components are incorrectly named for our Swedish domain specific words, so we need to either contort our view components names or manually write our own tag helpers (because HtmlTargetElement doesn't work on view components).

It's manageable but it's unnecessary friction and surprising when it doesn't work as it's supposed to.

@jjonescz
Copy link
Member Author

Let me reopen the original issue with this alternative so it will get triaged again.

@danroth27
Copy link
Member

@DavidZidar Instead of using View Components, could you use Blazor components with the Component Tag Helper instead? If not, what would be needed to enable you to do so? Ideally, we'd like to align ASP.NET Core on a single component model.

@DavidZidar
Copy link

@danroth27 We have a fairly large application that is built on top of Optimizely CMS where all the Optimizely Blocks are view components (because Optimizely requires this), so we use view components for non-block components as well to be able to reuse a lot of common code. I may be wrong but I don't think HTML helpers and tag helpers work in Blazor components?

Also, we have a couple of filters that need to wrap view components (to perform error handling and authorization policies and so on) so I had to find a way to make that work with view components because that is not something that is supported by ASP.NET by default. Can this be done with Blazor components? This is not something that I have investigated.

@danroth27
Copy link
Member

I may be wrong but I don't think HTML helpers and tag helpers work in Blazor components?

That's correct.

Also, we have a couple of filters that need to wrap view components (to perform error handling and authorization policies and so on) so I had to find a way to make that work with view components because that is not something that is supported by ASP.NET by default. Can this be done with Blazor components?

Nope, this also isn't something we currently support.

Thanks for sharing these additional details!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"HTML" name for ViewComponents and TagHelpers not generated correctly if class name contains extended letters
5 participants