Skip to content

Conversation

VelizarD
Copy link
Contributor

@VelizarD VelizarD commented Sep 2, 2025

Since mode prop is definitely treated as an optional:

export default function register(Component, tagName, propNames, options) {
	function PreactElement() {
		const inst = /** @type {PreactCustomElement} */ (
			Reflect.construct(HTMLElement, [], PreactElement)
		);
		inst._vdomComponent = Component;
		inst._root =
			options && options.shadow
				? inst.attachShadow({ mode: options.mode || 'open' })
				: inst;
		return inst;
	}
	....

this behaviour should be reflected in the typedef as well since v4.4.0 is breaking for TypeScript users.

@VelizarD VelizarD changed the title fix: expose correct typing fix: expose correct types Sep 2, 2025
Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you mean "treated", not "threaten" (that has a very, very different meaning), but I don't think this is what we want to do; if anything, the runtime behavior should change in the next major.

If users are using shadow DOM then they really should be explicitly stating the mode. I think it's better if our types lie here a bit as it forces users onto better, more explicit patterns.

@VelizarD
Copy link
Contributor Author

VelizarD commented Sep 2, 2025

I assume you mean "treated", not "threaten" (that has a very, very different meaning), but I don't think this is what we want to do; if anything, the runtime behavior should change in the next major.

If users are using shadow DOM then they really should be explicitly stating the mode. I think it's better if our types lie here a bit as it forces users onto better, more explicit patterns.

Yep treated was what I meant, anyways to force users to pass the mode isn't bad at all but this should have been done via a breaking change IMHO.

@rschristian
Copy link
Member

Unfortunately we don't control the TS team's roadmap, this causing a breakage was out of our control. The intention has always been pretty clear though, even if at runtime this is massaged over.

@VelizarD
Copy link
Contributor Author

VelizarD commented Sep 2, 2025

What do you mean by control the TS team roadmap? The typedef is in your control and changing it like i've done results in correct and non breaking typing for TS users:
image

If you want to change the behaviour and be more explizit (which is always a good idea) then you have to introduce it with a breaking change.

@rschristian
Copy link
Member

since v4.4.0 is breaking for TypeScript users.

I'm pretty sure the types were written prior to this; we didn't make any breaking change ourselves, to my knowledge.

If anything, it was a bug/lack of a feature in TS that allowed you to get away without specifying in the past which we'd generally call acceptable.

@VelizarD
Copy link
Contributor Author

VelizarD commented Sep 2, 2025

What about users which used preact-custom-element without the @types/preact-custom-element types, updating to v4.4.0 would still break for them?

@rschristian
Copy link
Member

Oh you're saying preact-custom-element v4.4, not TS v4.4? That was not made clear.

Let me take another look, but it'll be a few hours.

@VelizarD
Copy link
Contributor Author

VelizarD commented Sep 2, 2025

OK, I’ll take the blame for this misunderstanding — it seems the language barrier is bigger than I had feared. If you ever find yourself in Austria, I’m definitely inviting you for a few beers! 🍻 😆

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right and I screwed up, sorry!

I obviously misunderstood what version you were referring to (I assumed it was TS and a change in how they handle types), but also when I was investigating, I saw #85 and mistakenly opened up the commits list rather than viewing the files overall... the PR author originally added a .d.ts file before switching it over to JSDoc, and I incorrectly interpretted that as us always having a .d.ts file.

Sorry for the mix-up and I appreciate you bearing with me 😅

@rschristian rschristian merged commit 859566d into preactjs:master Sep 3, 2025
1 check failed
@rschristian rschristian mentioned this pull request Sep 6, 2025
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