Props spread incorrectly overrides explicit attribute definitions

[samuelhulla]

Astro Info

Astro                    v4.15.2
Node                     v20.7.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             @astrojs/tailwind

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

With a super simple component definition

---
// src/components/Broken.astro
import type { HTMLAttributes } from 'astro/types'
import { cn } from '@/lib/style' // pretty much what clsx does

export type Props = HTMLAttributes<'div'>

const props = Astro.props
---

<div {...props} class={cn("bg-lime-300", props.class)}>
  <slot />
</div>

Also as a side-note I know you can use class:list directive which has clsx built in, but i just found it easiest to demonstrate this behaviour with styles so I just used this as it's from a real-life use-case for a low-code component library i'm authoring, since in my scenario I'm sometimes using more complex styling imports than just the base clsx for stuff like variants and whatnot.

Now let's consider a simple page usage

---
// pages/page.astro
import Broken from '@/components/Broken.astro'
---

<Broken>this works</Broken>
<Broken class="py-4">this doesnt</Broken>

The first instance of the component works fine and renders with the background as expected. However the second one with class="py-4 does not merge as expected, since the {...props} spread overrides the later defined class definition.

How to fix this (currently available workaround):

In order to fix this behaviour, we'll need to destructure the conflicting attributes i.e.

---
// src/components/Workaround.astro
import type { HTMLAttributes } from 'astro/types'
import { cn } from '@/lib/style'

export type Props = HTMLAttributes<'div'>

const { class: className, ...props } = Astro.props
---

<div {...props} class={cn("bg-lime-300", className)}>
  <slot />
</div>

Had to rename class to className in the destructuring, since class is a reserved keyword in js.

Now in my page definition

---
import Workaround from '@/components/Workaround.astro'
---
<Workaround>this works</Workaround>
<Workaround class="py-4">this also works</Workaround>

This fixes the issue, where the classes merge succesfully with the bg-lime-300 and py-4 since we destructured the attribute class and the {...props} rest arg no longer overrides it.

Now we fixed the issue, but obviously this is very error prone if a developer introduces any further custom/explicit attribues in the future, they will also need to be explicitly destructured, otherwise they will be overriden. This is an error-prone behaviour in my opinion.

What's the expected result?

I would expected the attribute spread to be compiled / built correctly, respecting the order of the spread operator and it's attribute definitions, aligning the behaviour with other frameworks like react, solid, vue, etc.

import { cn } from '@/lib/style'
import type { DetailedHTMLProps, HTMLAttributes } from 'react'

export type Props = DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>

export default function WorksOutOfTheBox(props: Props) {
  return (
    <div {...props} className={cn("bg-lime-300", props.className}>{props.children}</div>
  )
}

This works out of the box, the {...props} do not override className definition, since the className definition takes precedence in definition order over the spread operator.

<WorksOutOfTheBox>this works</WorksOutOfTheBox>
<WorksOutOfTheBox className="py-4">this also works</WorksOutOfTheBox>

Aligning with the same spread operator behaviour as in javascript objects, i.e.

{
  ...obj,
  foo: 'bar'
}

Where if obj had a key foo, it would be overriden with the value of bar, since the key foo was manually defined after the spread operator happened.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-astro-uen4d4?file=src%2Fcomponents%2FBroken.astro

Participation

• I am willing to submit a pull request for this issue.

[mayank99]

@samuelhulla Thank you for opening this issue! I have been meaning to report this since the very early days of Astro, but a combination of procrastination and low spoons means I've been working around it for literal years at this point 😅

I should note that a similar issue with likely the same root cause is that spread props do not override the explicit attributes that come before it. This pattern, when it works, can be useful when you want to set some default attribute values that should be overridden by spread props. (I use this pattern all the time in JSX)

<!-- `type` prop passed to the component should override `type="button"` -->
<button type="button" {...Astro.props}>
  <slot />
</button>

See minimal repro.

Fixing this would be a breaking change, so this should probably go in 5.0.

[matthewp]

Ah, so HTML doesn't work how I would expect here. Overridden props do come after previous ones, but because they all appear in the HTML, the browser actual uses the first value as the value, not the last, which is what I would expect. Here's a simpler example of what gets rendered: https://codepen.io/matthewp/pen/abggJgy

So fixing this will be a compiler change, unfortunately. I think what we'd have to do is if there is a spread in the attributes, move any static values into the spread function rather than treat them separately, as we currently do.

[samuelhulla]

@matthewp I see. Kind of begs the question wether astro should adhere more closely to the native HTML behaviour or align it's behaviour more to the js-land of rendering frameworks.

I still think it's bit better to have it aligned more with the latter option, given a lot of Astro users do come from the React/Vue/Svelte/Solid land, but it's ultimately your call guys.