search

patternfly / patternfly-elements

PatternFly Elements. A set of community-created web components based on PatternFly design.
https://patternflyelements.org/
MIT License
377 stars 92 forks source link

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

Open heyMP opened 2 weeks ago

heyMP commented 2 weeks ago

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)

bennypowers commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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);