ungap / custom-elements

All inclusive customElements polyfill for every browser
ISC License
240 stars 14 forks source link

upgrade() seems to not work as expected #11

Closed SkidX closed 2 years ago

SkidX commented 2 years ago

Hi, It's about custom built-in elements support in Safari. I think the upgrade() in this polyfill is not working as expected, while your other specific polyfill ( https://github.com/ungap/custom-elements-builtin ) works correctly (by this I mean it gives the same result as browsers with native support).

I would be more than happy just using the other polyfill since I have to support only modern browsers and Safari is the only one not compliant with the standard, but that one is deprecated.

here is the code to reproduce the issue (I don't have a mac, so I'm testing everything through Browserstack)

class TestImg extends HTMLImageElement {}

const element = document.createElement('img', {is: 'test-img'});
console.log('before define', element instanceof TestImg); // false, as expected

customElements.define('test-img', TestImg, {extends: 'img'});
console.log('after define', element instanceof TestImg); // false, as expected

customElements.upgrade(element);
console.log('after ugprade', element instanceof TestImg);
// true on browsers with native support and using @ungap/custom-elements-builtin, 
// but false when using @ungap/custom-elements

Cheers

WebReflection commented 2 years ago

Safari is the only one not compliant with the standard, but that one is deprecated.

nothing is deprecated. It's specs and stays in specs not deprecated.

// true on browsers with native support and using @ungap/custom-elements-builtin, // but false when using @ungap/custom-elements

this is funny because this polyfill uses @ungap/custom-elements-builtin but I'll have a look.

Also please consider giving vanilla-elements a chance!

// optionally use import-map to differentiate Safari from others
import {define, HTML} from 'vanilla-elements/poly';

class TestImg extends HTML.Image {}
define('test-img', TestImg); // no need to use {extends}

This tiny helper uses @ungap/custom-elements-builtin only and it fixes a long standing bug with attributes in construction

SkidX commented 2 years ago

nothing is deprecated. It's specs and stays in specs not deprecated.

Sorry, I meant your other polyfill is declared as deprecated, not the spec.

this is funny because this polyfill uses @ungap/custom-elements-builtin but I'll have a look.

Thanks a lot!

Also please consider giving vanilla-elements a chance!

Looks nice, I'll check it.

WebReflection commented 2 years ago

I see ... my misunderstanding then, as I've mentioned this one, not the old one. Then vanilla-elements won't fix it neither, I need to check @webreflection/custom-elements-builtin module instead. Will try when possible!

SkidX commented 2 years ago

I tried now that one ( @webreflection/custom-elements-builtin ) and that is even more strange, since it is printing false also on Chrome (where it should not do anything, I would expect).

Anyway, thanks again for checking it, I will use @ungap/custom-elements-builtin in the meanwhile, no hurry.

WebReflection commented 2 years ago

Turns out ... there was no upgrade for builtin extends, as I've never needed that use case and completely forgot about it.

Now there is upgrade and tests are fine too and work as expected like native implementations.

SkidX commented 2 years ago

Great, thanks again!