Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mistake in source code #20

Closed
kapouer opened this issue Oct 25, 2023 · 15 comments
Closed

Mistake in source code #20

kapouer opened this issue Oct 25, 2023 · 15 comments

Comments

@kapouer
Copy link

kapouer commented Oct 25, 2023

https://github.com/WebReflection/custom-elements/blob/0acfb63eab710719e84ba8773f0af689f091dbfd/template/index.js#L9
@WebReflection
Copy link
Member

it's here indeed https://github.com/ungap/custom-elements/blob/master/index.js#L373

@WebReflection
Copy link
Member

do you have any specific problem though? I think current state works with both old and newer Safari and the fix went in and it got tested to work everywhere ... I am saying this because while I fully agree there is a typo, I don't know enough of their internal changes to be sure if I change that, we'll have new issues on Safari.

@kapouer
Copy link
Author

kapouer commented Oct 25, 2023

No I have nothing to say more :) just that it's a bug waiting to happen, probably.

@WebReflection
Copy link
Member

AFAIK so far no browser complained, but I hear you ... although things work now. I will eventually fix this and push major (or minor, if I am still behind 0) so let's hope that won't crash the world once that happens.

@kapouer
Copy link
Author

kapouer commented Oct 25, 2023

Unfortunately I recently canceled my browserstack subscription. Do you know you can ask them to give this project free open source subscription ?

@WebReflection
Copy link
Member

I have one already, that's why I've said, everything is fine.

@kapouer
Copy link
Author

kapouer commented Nov 2, 2023

Since epiphany-browser is kind of webkit, I gave it a try...
@ungap/custom-elements 1.2.0 works there, but not version 1.3.0.

@WebReflection
Copy link
Member

Imagine I use GNOME Web as my daily driver

@kapouer
Copy link
Author

kapouer commented Nov 2, 2023

Well then that's odd. I'm diffing v1.2.0 / v1.3.0 to see what broke my setup

@kapouer
Copy link
Author

kapouer commented Nov 2, 2023

Ha damn again it's a problem on my side. I'm loading the polyfill somewhat too late.

@kapouer kapouer closed this as completed Nov 2, 2023
@kapouer
Copy link
Author

kapouer commented Nov 3, 2023 •
edited
Loading

FWIW my issue was that I was detecting the need to load the customElement polyfill using this code:

'customElements' in window && (function() {
 try {
  const BR = class extends HTMLBRElement {};
  const is = 'extends-br';
  customElements.define(is, BR, { extends: 'br' });
  return document.createElement('br', {is}).outerHTML.indexOf(is) > 0;
 } catch(e) {
  return false;
 }
})()

Which makes the corresponding detections fail in @ungap/custom-element, because extends-br is already registered.
This code should really clean after itself. EDIT: WICG/webcomponents#754 it cannot.

@WebReflection
Copy link
Member

WebReflection commented Nov 3, 2023 •
edited
Loading

I was detecting the need to load the customElement polyfill using this code:

The best way to run this polyfill for Safari or WebKit only is to stick this inline script as the first one on any page (within the <head>, of course):

<script>
if (!(self.chrome || self.netscape))
  document.write('<script src="https://cdn.jsdelivr.net/npm/@ungap/custom-elements/es.js"></'+'script>');
</script>

arguably you can use the same trick to bring in just https://cdn.jsdelivr.net/npm/@webreflection/custom-elements-builtin/es.js without even needing to feature-detect as it's clear at this point Apple won't ever implement builtin extends.

@WebReflection
Copy link
Member

CodePen Live Demo https://codepen.io/WebReflection/pen/wvNzOmr?editors=1000

@kapouer
Copy link
Author

kapouer commented Nov 3, 2023

Thanks for the additional infos. However I'm loading a bunch of other polyfills (mostly from polyfill.io's detectSource fields) before actually executing my library. It's a bit convoluted but it avoids user-agent detection, and all polyfills are equal :)

@WebReflection
Copy link
Member

I haven't used user agent sniffing, I've used proprietary fields per browser ... I am not parsing a string, I am checking the env 😉

Like I've said, that is the best way to land this poly for Safari only and no network request even ever happens in Chrome or Firefox ... the CDN caches that too so it's irrelevant for your users.

In short, do what you want, but the best wayfor this polyfil is the one I've suggested there, actually the CodePen example is even better because it doesn't trash any entry in the customElements registry at all so it never conflicts, not even if other projects embed this polyfil as the polyfil will pass the feature detection after the first patch so ... it's all good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@WebReflection @kapouer