Skip to content

Conversation

@samuelhulla
Copy link

@samuelhulla samuelhulla commented Dec 23, 2025

Changes

Adjusts astro compiler to adhere to JSX semantic prop overrides.

<!-- user html pre-compilation  -->
<div a="1" a="2" /> 

<!-- Currently results in ❌ -->
<div a="1" />

<!-- After this PR ✅ -->
<div a="2" />

Manually overriding like that is a rare occurance, but consider this far more common issue

---
const props = { class: "foo" } // obv. normally we'd use Astro.props but for keeping example concise
---
<!-- user html pre-compilation -->
<div {...props} class="bar" >

<!-- What you end up getting ❌ -->
<div class="foo" />

<!-- What most users expect they will get -- and will get after this PR ✅ -->
<div class="bar" />

Testing

Added both ts and go tests for testing the compiler (parser/printer) for deduping various example and cornercases like svg namespaces, multiple spreads, etc.

Tested via standard test workflow:

pnpm build:all
UPDATE_SNAPS=true go test -v ./internal
pnpm test

Docs

Doesn't require any direct documentation changes imho, but does require a breaking change notice in the changelog / announcement post of the version (even though majority of users should be unaffected by this change)

Closing remarks

Full disclosure, I have very limited golang experience. I did my best to address the necessary changes in a meaningful and performant way, but I wanted to make it explicitly clear -- My expertise is TypeScript with a hobbylang of Rust, so just so we're on the same page -- I did use AI to help me get this PR over the finish line to make up for my lack of go knowledge.

I did my best to verify everything and write a comprehensive test suite for it, but it is more than likely that there may be something suboptimal given the scope of the compiler, the fact that it's my first change and that I have very limited go experience from a single sideproject ~2 years ago.

Either way hopefully this should serve as a solid starting point and wanted to deliver you guys'n'gals bit of an early christmas present for all the hard work you've been putting in all this time. Have wonderful holidays 🎄

@changeset-bot
Copy link

changeset-bot bot commented Dec 23, 2025

⚠️ No Changeset found

Latest commit: 108239d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@samuelhulla samuelhulla changed the title Fix/semantic props override Fix: semantic props order override Dec 23, 2025
@matthewp
Copy link
Contributor

matthewp commented Jan 7, 2026

This looks great overall. Will review with the team to see if we can get into 6.0.

@samuelhulla samuelhulla force-pushed the fix/semantic-props-override branch from 665a6e2 to 108239d Compare January 8, 2026 09:18
@samuelhulla
Copy link
Author

Hey @matthewp I reverted (rebased) the changeset changes as per your request.

However, the command you suggested (pnpm run changeset) does not exist in the scripts of root level package.json nor of the packages/compiler/package.json so it fails.

Am I supposed to run pnpm dlx changeset without any specific script or is there some other customized script you guys have in mind.

@matthewp
Copy link
Contributor

matthewp commented Jan 8, 2026

Ah yeah sorry, I guess we didn't add the script to the package.json. pnpm dlx changeset or just pnpm changeset should both work I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants