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.

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

GitHub: "Error: A custom element with name 'toggle-switch' has already been defined." #46

Closed
Vangelis66 opened this issue Sep 12, 2022 · 4 comments

Comments

@Vangelis66
Copy link

Browser: Serpent 52.9.0 (2022-08-05) (32-bit)
Extension version: Latest master HEAD (palefill-v1.20-2-git-20220907-g5c251f5)

Description

Over the last few days, when browsing any GitHub page, I couldn't help noticing the below Web Console error:

Error: A custom element with name 'toggle-switch' has already been defined.

The error is generated by GH script:

https://github.githubassets.com/assets/compat-838cedbb.js

Screengrab attached:

GHerror

Is this something to worry about?
Can it be eliminated?

Thanks for your stupendous, hard, work! 👍

@SeaHOH
Copy link

SeaHOH commented Sep 13, 2022

      customElements.define = function (name, cls) {
--      asNames.add(name);
        cls.prototype.addEventListener = addCEEventListener;
        cls.prototype.removeEventListener = removeCEEventListener;
++      if (asNames.has(name)) return;
++      asNames.add(name);
        oldCED.call(customElements, name, cls);
      };

@Vangelis66
Copy link
Author

Thanks @SeaHOH 👍 ; your suggested patch (in file ./lib/polyfills.js) appears to "take care" of the reported error successfully! 🎉

--- polyfills.js.OLD    2022-09-07 23:00:16.000000000 +0300
+++ polyfills.js.NEW    2022-09-13 17:53:07.303486000 +0300
@@ -406,9 +406,10 @@
       rel.call(target, type, listener, options);
     }
     customElements.define = function (name, cls) {
-      asNames.add(name);
       cls.prototype.addEventListener = addCEEventListener;
       cls.prototype.removeEventListener = removeCEEventListener;
+      if (asNames.has(name)) return;
+      asNames.add(name);
       oldCED.call(customElements, name, cls);
     };
   }

@martok
Copy link
Owner

martok commented Sep 14, 2022

Very busy with work currently, work here will be slow for the forseeable future.

Can someone check the specification what the correct behavior should be? Obviously that patch suppresses the errors, but doing that when the original (or polyfill) actively throws seems wrong to me. Maybe something was changed and it isn't an error anymore, so frameworks just do it?

@martok
Copy link
Owner

martok commented Oct 3, 2022

So it was a standards issue, but in a different way than I expected.

The official polyfill throws errors as instances of the JS SyntaxError and Error classes. This is wrong, they should be DOMException subtypes. Something deeeeeep in Github's framework relies on that to detect and handle errors. This is the correct way, but doesn't work with the polyfill.

I'm fixing that on the attachShadow side even though it technically shouldn't be there since many more pages provide local copies of the customElements polyfill, so it's easiest to catch where we often attach anyway.

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

No branches or pull requests

3 participants