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

[feat] Don't throw error on duplicate custom element definition #2843

Open
heyMP opened this issue Sep 13, 2024 · 4 comments
Open

[feat] Don't throw error on duplicate custom element definition #2843

heyMP opened this issue Sep 13, 2024 · 4 comments
Labels
feature New feature or request priority: low Severity level: 3

Comments

@heyMP
Copy link
Contributor

heyMP commented Sep 13, 2024

Description of the requested feature

At scale, it's very difficult to prevent custom elements from being defined more than once. When lots of teams are trying to publish their features to a websites, it's very difficult to keep those custom element dependencies deduped.

I'm proposing that we create our own customElement decorator that wraps customElement.define in a try/catch. That try/catch can detect the custom element duplication error and convert it into a console.warn message.

Impacted component(s)

  • all components
@heyMP heyMP added feature New feature or request priority: low Severity level: 3 labels Sep 13, 2024
@heyMP heyMP linked a pull request Sep 13, 2024 that will close this issue
@heyMP heyMP changed the title Don't throw error on duplicate custom element definition [feat] don't throw error on duplicate custom element definition Sep 13, 2024
@heyMP heyMP changed the title [feat] don't throw error on duplicate custom element definition [feat] Don't throw error on duplicate custom element definition Sep 13, 2024
@bennypowers
Copy link
Member

while it seems at first blush that this is a good solution it actually will produce incorrect behaviour down the line

let's say we implemented this in pfe 5, then released pfe 6, and a user ended up loading version 5 first then version 6 later. They would be surprised to see that their html does not work (because it's using version 6's apis against version 5 code)

the comprehensive solution to this problem is scoped custom element registries. There is a stable polyfill for that, but adopting it is non-trivial, so it's better to wait for browser support. On the plus side there are WPT for this feature so maybe it will make it into baseline 2025.

In the mean time we have to cope with the global custom element namespace. We might consider adopting versioned prefixed like <pf-v5-card> or some such.

@heyMP
Copy link
Contributor Author

heyMP commented Sep 16, 2024

Yeah, I totally agree that this is not a solution that something like scoped custom registries or version namespaced elements. This proposal would be more of a stop-gap as outlined in that document, Alternatives to allowing multiple definitions

Correctly deduping all dependencies for lots of teams shipping JS bundles to the same page has been a challenge and there are always new has already been used with this registry unhandled errors. As a developer that has no control over other teams code, I would prefer the duplicate definition errors to be handled instead of potentially breaking my entire app.

@bennypowers
Copy link
Member

I think this is going to need some focused attention on a case by case basis. I'm really wary about shipping a solution for the right-now problem which will be forgotten about by the time tomorrow's problem shows up.

Apps should be in charge of their own bundles

Pages that have multiple teams involved in an asynchronous cycle should use import maps, and the version line should be the private fiefdom of the team lead for that domain. Like a benevolent dictator, they should have final say on which resources get loaded, but contributing/downstream teams should have control over when they get loaded. That's what import maps provide and it's where we should aim, while cognizant that there still remains the need to coordinate on the major-version-line

For example:

A Drupal app provides an import map using one of the off-the-shelf modules

On one of those Drupal pages, a microfrontend team targeting that specific Drupal instance needs to load library modules for their dynamic client-side app.

At the same time and on the same page, a marketing team needs to load dynamic content targeting any domain in the entire organization.

The microfrontend team can coordinate directly with the owner of the page <head>. Satisfaction is but a short phone call away.

The marketing team, who have to inject the same campaigns dynamically into multiple projects each having their Iestyn version line, must own and maintain a map of project-owner-to-version-line, and adjust the content they inject according to the context they inject it into. So they know that Bob is stuck on version 1 while Sally (that keener) is already on version 3 - they maintain two separate templates for their campaign and serve the appropriate one to the domain in question.


SCER will solve all of this nicely, but im personally not yet convinced that the price of the polyfill is worth it, yet.

@heyMP
Copy link
Contributor Author

heyMP commented Sep 17, 2024

As an alternative to the try/catch block around the customElement define decorator, how would you feel about separating the customElement defines into a separate side effect file?

Example:

RhButton.js
rh-button.css
rh-button.js (side-effect)

rh-button.js

import { RhButton } from './RhButton.js';
customElement.define('rh-button', RhButton);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request priority: low Severity level: 3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants