ungap / custom-elements

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

` customElements.whenDefined()` not working for custom elements already defined before Polyfill is loaded #21

Open mbehzad opened 11 months ago

mbehzad commented 11 months ago

Hi, Image this scenario where there are a lot of custom elements and only a small part of them would extend build in elements, Safari would support the first ones but need the Polyfill for the build in extenders. I would prefer to lazyload the Polyfill only when needed and define only the CEs relaying on it after the Polyfill is loaded but the other ones define right away.

The problem is that to my understanding, for example, customElements.whenDefined is overridden and will only know about CEs that are defined after that point.

I could open a PR where the copy of the original customElements.whenDefined is kept and i.e. used as return Promise.any([_defined.get(name).$, _originalWhenDefined(name)]);.

Does this make sense? is there anything else that could break? or supporting loading the pollyfill later for whenDefined brings the expectation that it also works with other APIs and that creates more confusion for the users?

cheers Mehran

WebReflection commented 11 months ago

the polyfill patches more than just builtin extends but fair enough ... I think it migh tbe about the time to re-think the polyfill and patch builtin extends only instead as whenDefined now resolves with the class as argument across the browsers (IIRC) and effectively only builtin extends are needed these days as polyfill, thanks to IE being dead long time ago.

So yes, having an any for lazy polyfill inclusion makes sense but here my counter-question: would you also use a single module/entry point to eventually also decide to land the poly or not?

@ungap/custom-elements-needed which default export returns true or false so that you are in charge to load eventually later on the poly that will patch/fix only builtin extends?

mbehzad commented 11 months ago

Hi Andrea, I'm not sure if i understand the question correctly, but my use case would be something like:

// teaser.js
class Teaser extends HTMLElement {
}

// form.js
 if (!supportsBuiltinExtension) await import("@ungap/custom-elements");

class Form extends HTMLFormElement {
}

And these components are loaded independently from each other (imagine something like the old Polymers HTML imports but with js modules) and you don't really want to block the Teaser component from being defined by waiting for the poly that is only needed by the the other component.

WebReflection commented 11 months ago

I've been thinking about this:

Promise.any([_defined.get(name).$, _originalWhenDefined(name)]);

there is an issue ... it creates either ways a forever pending promise, either on the builtin registry or the polyfill one.

The hint here is that the polyfill should be loaded ASAP on top of any page needing it, which is fairly trivial:

<!-- top most page script for Safari only polyfill -->
<script>
self.chrome ||
self.netscape ||
document.write('<script src="//unpkg.com/@webreflection/custom-elements-builtin"><\x2fscript>');
</script>

Why can't this be your solution, if I might ask?

mbehzad commented 11 months ago

sure that would work. but having the script render blocking has its down sides, or even deferred would mean you are restricted by not being able to async load other scripts or enter some race conditions, and with all the frameworks, bundlers and CMSs out there that might be even difficult in practice to create the correct script tag at top of html (I know, not the polys fault). or if these are from some component libraries, 3rd party widgets, ... people would want to load all the code via their js. or when they are loaded as part of some SPA's route navigation and way after the page and its html was initially loaded.

Regarding the pending promise, that wouldn't be a catastrophe, it happens all the time (even right now the poly doesn't remove the stored promise in _defined to allow for its garbage collection) But if loading the poly after some custom elements are already defined would break its other functionalities, then i understand there can't be done much. But it would've solved my problem.

WebReflection commented 11 months ago

Regarding the pending promise, that wouldn't be a catastrophe, it happens all the time (even right now the poly doesn't remove the stored promise in _defined to allow for its garbage collection)

it can't be removed ... anyone else later on might ask for the same whenDefined custom element and the same goes for the native builtin ... maybe I am overlooking at that forever pending issue as the native behavior is identical when any component never gets a chance to resolve (i.e. classes defined with parent/outer dependencies in order to work properly) ... so here my thinking:

In latter case, that entry point will use the same code but it will guarantee that whenDefined is used, or even get or getName, returns components from either the native, previous, definition, or the one patched ... does this sound like a good idea to you?

WebReflection commented 11 months ago

... heck I might even call the poly @ungap/custom-elements-webkit at this point, but that feels like a direct attack to the team (it is!) so maybe something smoother to digest for everyone would be a better name ... hints welcome for the name.

mbehzad commented 11 months ago

yes i think, in case it would be possible to only import the polyfill parts the user needs (mostly the build in extension), that would be really great. Regarding the naming, of course infamously that is the hard thing about programming, but if i had to pick, I would've said the same npm package but with different exports sub paths. e.g. / or @ungap/custom-elements would have all the polyfills, @ungap/custom-elements/builtin only the code needed for the builtin extension. and any future addition to the web components spec that takes Safari another 10 years to implement under some other paths related to those features. and if under the hood some relay on the others, they can import them and at the end the user loads only the code / polyfills that they think they need.

WebReflection commented 11 months ago

OK, that might work ... although the doubt around a way, within the module, to feature detect if such poly is needed, is still a good idea ... otherwise people might just bring it by accident or in a non cross-browser friendly way ... what if that exports both needsPatch() and patch() methods so that tree-shaking can work out of the box?