fix: make sure unknown is mapped to HTMLUnknownElement cstr

[hoebbelsB]

What is it?

• Feature / enhancement
• Bug
• Docs / tests

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

The issue

Chrome maps correctly

WebKit browsers map other nodeNames (in this case DIRECTORY) to HTMLUnknownElement

Which results in the hardcoded UNKNOWN property being undefined in WebKit browsers, causing them to throw errors like a madman.

The reason

Webkit iterates over the possible APIs in a different order. It will receive directory as first nodeName which results in HTMLUnknownElement constructor. The function readOwnImplementation maintains a Set<string> with constructor names. So the first one to occupy the space for HTMLUnknownElement will be directory.

If you filter for the possible cstrImpls which have the HTMLUnknownElement constructor name, you'll get multiple results with different orderings, depending on the browser.

Broken ordering on webkit

Proper ordering on chrome

The fix

I've simply introduced a new filter that filters out all nodenames that will result in HTMLUnknownElement, but UNKNOWN. The worker will anyway fall back to HTMLUnknownElement, so there should be no change in functionality.

Checklist:

• My code follows the developer guidelines of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• Added new tests to cover the fix / functionality

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
partytown	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 25, 2024 10:58am

[hoebbelsB]

no idea why the test would fail because of my changes. They also fail locally for me when checking out the main branch

[gioboa]

I see, thanks @hoebbelsB for your help

[aaa123eee]

Hello @gioboa

Is there anything we can do to move it forward?

[gioboa]

Hi @aaa123eee, I'm sorry. I'm no longer in the project. You can contact the builder.io team in this Discord channel
Thanks

[aaa123eee]

Hi @aaa123eee, I'm sorry. I'm no longer in the project. You can contact the builder.io team in this Discord channel Thanks

Thanks for your reply and your contributions!

[aaa123eee]

Hi @aaa123eee, I'm sorry. I'm no longer in the project. You can contact the builder.io team on this Discord channel Thanks

Unfortunately this link does not work for me

Is there anybody I can ask for a proper one? probably @adamdbradley @mhevery

[aaa123eee]

there's a related issue about that #547

maybe @hamatoyogi knows 🤔

Unfortunately this link does not work for me

Is there anybody I can ask for a proper one? probably @adamdbradley @mhevery

[gioboa]

@aaa123eee can you try this one please?

[aaa123eee]

@aaa123eee can you try this one please?

this invite works well! thank you!

[hamatoyogi]

there's a related issue about that #547

maybe @hamatoyogi knows 🤔

Unfortunately this link does not work for me
Is there anybody I can ask for a proper one? probably @adamdbradley @mhevery

Hey, I'm no longer at Builder, but maybe if i tag @steve8708 something will happen 😅

[kylefowler]

Thanks for the PR, we will have a look at this next week.

[kylefowler]

I think if we can get a unit test on the change to verify behavior following our contribution guides we should be good to get this merged in. thanks for the patience here. https://github.com/BuilderIO/partytown/blob/main/CONTRIBUTING.md#submitting-issues-and-writing-tests

[gioboa]

Thanks @hoebbelsB for your commitment and patience 🙏
Can you add a test for this specific case and flip the double ! condition in the filter please? ( it's not that readable in this way )

[hoebbelsB]

yo, cool to see you back. will do as soon as I find time! Hopefully this week

[gioboa]

@hoebbelsB is this PR still valid?

[aaa123eee]

@hoebbelsB is this PR still valid?

it definitely is!

[gioboa]

@aaa123eee Do you also have this error? if yes, can you check this change to verify it please?

I've simply introduced a new filter that filters out all nodenames that will result in HTMLUnknownElement, but UNKNOWN.

@hoebbelsB based on your description I changed the filter logic.
is it correct? remove the HTMLUnknownElement with UNKNOWN and keep the others

return (constructorName !== 'HTMLUnknownElement' || elm.nodeName.toUpperCase() !== 'UNKNOWN');

[hoebbelsB]

@gioboa no, it's not the same as before.

I've also worked on a test to proof it. I've double checked the previous, my solution and your solution.
With previous (main), test fails on webkit only.
With your solution the test also fails on chromium & webkit.
With my solution the test passes.

[gioboa]

Thanks @hoebbelsB for your help
It looks good to me