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.

Sign up for GitHub

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

Jump to bottom

Make duplicate attribute names an error #604

Merged
merged 3 commits into from
May 21, 2019
Merged

Make duplicate attribute names an error #604

merged 3 commits into from
May 21, 2019

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented May 20, 2019 •
edited
Loading

This came up in the context of discussion around #dotnet/aspnetcore#5071.

Browsers use a 'first attribute wins' rule for evaluating attributes and
building the DOM - though duplicate attributes are considered to be a
spec violation.

Blazor wants to use 'last attribute wins' rules when dynamically building the attribute list because that's most sensible when you consider evaluation order and splatting.

This means that we have to have the markup block pass do the same thing
as the Blazor runtime when duplicates appear statically.............. OR since this is a
spec violation I'm proposing that we just make it an error (like we do
for malformed tags). There's no useful purpose for duplicate attributes
since browsers ignore it.

@rynowak
Copy link
Member Author

rynowak commented May 20, 2019

/cc @SteveSandersonMS @danroth27 any objections?

@rynowak rynowak changed the title Make dupliate attribute names an error Make duplicate attribute names an error May 20, 2019
@SteveSandersonMS
Copy link
Member

This looks great to me.

Following the conversation on #10357, could we go further and treat explicit duplicates as errors on components and non-markup elements too?

@rynowak
Copy link
Member Author

rynowak commented May 20, 2019

I think that's probably the direction we're headed in.

I can also do something SUPER DOPE and make it so that <input type="text" bind="@someText" onchange="@changed" /> reports an error and explains that it was caused by the bind attribute.

@rynowak rynowak force-pushed the rynowak/dupliate-attribute-name branch 2 times, most recently from 527601a to 684e8ac Compare May 21, 2019 20:25
@rynowak
Copy link
Member Author

rynowak commented May 21, 2019

@ajaybhargavb @NTaylorMullen - can you take a look?

Make dupliate attribute names an error
bfaffaf 
This came up in the context of disucssion around #5071.

Browsers use a 'first attribute wins' rule for evaluating attributes and
building the DOM - though duplicate attributes are considered to be a
spec violation.

Blazor wants to use 'last attribute wins' rules because that's most
sensible when you consider evaluation order and splatting.

This means that we have to have the markup block pass do the same thing
as the Blazor runtime when duplicates appear. However, since this is a
spec violation I'm proposing that we just make it an error (like we do
for malformed tags). There's no useful purpose for duplicate attributes
since browsers ignore it.

Since we're not going to allow this for elements. Let's go all of the
way and block it for components as well. And while we're here, lets
block some of the invalid cases where users try to mix onchange and
bind. These don't work today, so we should make them an error. If we
decide to add support for this case to the runtime that would require
compiler work anyway.
@rynowak rynowak force-pushed the rynowak/dupliate-attribute-name branch from 684e8ac to bfaffaf Compare May 21, 2019 20:37
@rynowak
Copy link
Member Author

rynowak commented May 21, 2019

If we're going to be using the terminology of "parameter" for bind-ABC:XYZ's ABC @ajaybhargavb we may want to consider renaming the directive attribute "parameters" (XYZ) to be "modifiers" to avoid confusion.

No, I'm referring to the Message in the following:

<MyComponent Message="" />

This error message is explaining that both Message and bind-Message set the component parameter Message. I don't think there's anything to fix here.

@rynowak
Copy link
Member Author

rynowak commented May 21, 2019

@aspnet/build - this guy looks like the PR validation is stuck.

@natemcmaster
Copy link
Contributor

https://status.dev.azure.com/_event/121971177

@rynowak
Copy link
Member Author

rynowak commented May 21, 2019

@natemcmaster - want to hit the like button on this PR for me?

@natemcmaster natemcmaster merged commit 0c59ca5 into master May 21, 2019
@ghost ghost deleted the rynowak/dupliate-attribute-name branch May 21, 2019 23:37
@rynowak
Copy link
Member Author

rynowak commented May 22, 2019

Thanks 👍

JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020
@rynowak @natemcmaster
Make duplicate attribute names an error (dotnet/razor#604)
95bb698 
This came up in the context of discussion around aspnet/AspNetCoredotnet/aspnetcore-tooling#5071.

Browsers use a 'first attribute wins' rule for evaluating attributes and
building the DOM - though duplicate attributes are considered to be a
spec violation.

Blazor wants to use 'last attribute wins' rules because that's most
sensible when you consider evaluation order and splatting.

This means that we have to have the markup block pass do the same thing
as the Blazor runtime when duplicates appear. However, since this is a
spec violation I'm proposing that we just make it an error (like we do
for malformed tags). There's no useful purpose for duplicate attributes
since browsers ignore it.

Since we're not going to allow this for elements. Let's go all of the
way and block it for components as well. And while we're here, lets
block some of the invalid cases where users try to mix onchange and
bind. These don't work today, so we should make them an error. If we
decide to add support for this case to the runtime that would require
compiler work anyway.
\n\nCommit migrated from dotnet/razor@0c59ca5
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
@rynowak @natemcmaster
Make duplicate attribute names an error (dotnet/razor#604)
73694c3 
This came up in the context of discussion around aspnet/AspNetCoredotnet/aspnetcore-tooling#5071.

Browsers use a 'first attribute wins' rule for evaluating attributes and
building the DOM - though duplicate attributes are considered to be a
spec violation.

Blazor wants to use 'last attribute wins' rules because that's most
sensible when you consider evaluation order and splatting.

This means that we have to have the markup block pass do the same thing
as the Blazor runtime when duplicates appear. However, since this is a
spec violation I'm proposing that we just make it an error (like we do
for malformed tags). There's no useful purpose for duplicate attributes
since browsers ignore it.

Since we're not going to allow this for elements. Let's go all of the
way and block it for components as well. And while we're here, lets
block some of the invalid cases where users try to mix onchange and
bind. These don't work today, so we should make them an error. If we
decide to add support for this case to the runtime that would require
compiler work anyway.
\n\nCommit migrated from dotnet/razor@0c59ca5

Commit migrated from dotnet/aspnetcore@95bb698c5a5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajaybhargavb ajaybhargavb ajaybhargavb approved these changes

@NTaylorMullen NTaylorMullen NTaylorMullen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rynowak @SteveSandersonMS @natemcmaster @ajaybhargavb @NTaylorMullen