dotnet / razor

Compiler and tooling experience for Razor ASP.NET Core apps in Visual Studio, Visual Studio for Mac, and VS Code.
https://asp.net
MIT License
493 stars 191 forks source link

Consider non-ASCII characters in HTML case regex #10866

Closed jjonescz closed 2 weeks ago

jjonescz commented 2 weeks ago

Fixes https://github.com/dotnet/razor/issues/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.

333fred commented 2 weeks ago

Note that this is technically a breaking change

What is something that would break with this change?

davidwengier commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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

danroth27 commented 2 weeks ago

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

DavidZidar commented 2 weeks ago

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 commented 2 weeks ago

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

DavidZidar commented 1 week ago

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 commented 1 week ago

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

danroth27 commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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!