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: Infer component registrations - its currently very manual #5423

Closed
Iva-01 opened this issue Nov 30, 2021 · 14 comments
Closed

feat: Infer component registrations - its currently very manual #5423

Iva-01 opened this issue Nov 30, 2021 · 14 comments
Labels
area:fast-element Pertains to fast-element feature A new feature status:needs-author-input The PR needs input from the author warning:stale No recent activity within a reasonable amount of time

Comments

@Iva-01
Copy link

Iva-01 commented Nov 30, 2021

🙋 Feature Request

We are currently required to register all msft and fluent components and their dependencies (e.g. msftCarouselCard, msftCarouselCardWrapper, etc.). This is a very manual process and open to bugs as we've seen these bugs in production due to missed registrations. In addition, we may also end up registering things that may not be needed any longer, so it's very tedious to keep things up to date.

🤔 Expected Behavior

Provide a way to infer what needs to be registered from the templates themselves. Don't leave things open to human error.

😯 Current Behavior

Currently, all registrations are done manually by checking what the component is using in its template and then checking what that component uses in its template and so on until there are no more dependent registrations exist. For performance reasons, we only try to register what is used, thus, all the registations are done at the experience level. We tried registering eveything up front, but that bloats our bundle sizes and degrades our performance tremendously.

💁 Possible Solution

Ideally this would just be done automatically by looking at what the template is using and then inferring from there.

🔦 Context

We are trying to avoid any further live site issues with missed registrations.

💻 Examples

Here is an example of our live site issue where a registration was missed and UX did not render properly (cards were overlapping)

image

@Iva-01 Iva-01 added the status:triage New Issue - needs triage label Nov 30, 2021
@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Nov 30, 2021

To get a little more concrete, let's imagine a sample template as follows:

<div>
  <custom-element-one>
    <custom-element-two></custom-element-two>
  </custom-element-one>
</div>

Assume that:

  • custom-element-one has been manually registered
  • custom-element-two has not been manually registered

What would you expect to happen?

@EisenbergEffect EisenbergEffect added area:fast-element Pertains to fast-element feature A new feature status:needs-author-input The PR needs input from the author and removed status:triage New Issue - needs triage labels Nov 30, 2021
@Iva-01
Copy link
Author

Iva-01 commented Nov 30, 2021

I would expect to only have to register what I have referenced in my template. If there are additional dependencies that customElementTwo has I would expect that to automatically register. Good example is me using fluentTooltip. I would only expect to register fluentTooltip and not fluentAnchorRegion.

@EisenbergEffect
Copy link
Contributor

Ok, so is this really about dependencies? If I use component A and internally it uses component B, then you don't want to have to worry about registering component B. It should just work because it's used by component A. Is that the idea?

@Iva-01
Copy link
Author

Iva-01 commented Nov 30, 2021

Yes that would be a great start and would reduce bugs as folks are not aware of all the internal dependencies. Ideally, we wouldn't have to register anything and it would all be inferred from the template itself, but that's a stretch goal and not sure if its feasable. This is just feedback I'm giving based on the live site issues we've seen with the manual registration process.

@nicholasrice
Copy link
Contributor

We discussed this case in the original RFC #3361 (comment) and there are some proposed solutions there. @EisenbergEffect do you still think the .withRegistry() implementation you're proposing there will address this?

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Dec 1, 2021

It's a good question. I need to think about this more because there are few things at play here:

  • Associating dependencies
  • Handling both forms of registration (decorator and registry func)
  • Upcoming custom element registries standard
  • Potential tag name/prefix changes

Is this something we could choose to support only via tagFor used in function factory templates? For example, maybe tagFor could work against the registry function and if there's no tag found, it could use the func to JIT register the component. This would require using tagFor everywhere though. If we had useRegistry, then we'd have a trick of putting that in fast-element if we want to also support the function registry approach, since that's part of foundation. I worry about perf with that as well, especially if we have to handle re-writing templates.

The quickest/easiest solution would be to enable tagFor to auto-register. I'm not sure if that would meet the need though.

@EisenbergEffect
Copy link
Contributor

I think one way to do something like this would look like this:

const registry = new CustomElementRegistry();

provideFluentDesignSystem()
  .withRegistry(registry) // Instead of using the global registry, uses our scoped registry.
  .register(
    fluentButton()
  );

const template = html`
  <fluent-button></fluent-button>
`.useRegistry(registry); // Instead of resolving components from global, resolved them from our scoped registry.

The scoped element registry feature hasn't landed yet though. I'm not sure if there's a way to polyfill it. I'm also not sure just yet how to harmonize this with our templates that don't know the final tag name. Needs more thought.

@chrisdholt
Copy link
Member

I'd love to see the tagFor scenario managed in some way as that would certainly improve developer experience for folks using Foundation elements which rely on Anchored Region for now (See #4655 above). That would also benefit library authors who build more composed components perhaps.

@EisenbergEffect
Copy link
Contributor

The trick is that we can't couple a specific final composed component to a template. We could associate our base components with templates, but registration cannot be automatically handled with that data. We could throw a descriptive error message though, informing developers that an implementation of base component X needs to be registered. That could be done at registration time, so it would fail fast at startup in most scenarios.

@EisenbergEffect
Copy link
Contributor

But the other trick with that even is that fast-element doesn't know about base components. So, we'd have to think very carefully about the shape of the APIs here so that fast-foundation didn't seep down into fast-element.

@chrisdholt
Copy link
Member

But the other trick with that even is that fast-element doesn't know about base components. So, we'd have to think very carefully about the shape of the APIs here so that fast-foundation didn't seep down into fast-element.

Yeah I think with Foundation most of these tightly coupled instances are going to be rare due to the intent here.

Not that it makes it easier, and it may not be the ideal - but I wonder if it's reasonable to add this to the component configuration? One would assume that someone using the template to construct a component knows what that template and class relies on. Could we enumerate the components which need to be registered as dependencies (where fancyButton is a component registration for <fancy-button>)? It doesn't solve every problem, but I do think it provides a solution for those who are creating composed components.

import { Card, cardTemplate as template } from "@microsoft/fast-foundation";
import { fancyCardStyles as styles } from "./fancy-card.styles";
import { myFancyButton } from "../fancy-button";

const myFancyCard = Card.compose({
    baseName: "fancy-card",
    template,
    styles,
    shadowOptions: {
        delegatesFocus: true
    },
    // below could also be an array...
    dependencies: myFancyButton
})

@EisenbergEffect
Copy link
Contributor

I think we'd have to make the registry functions expose some information about themselves, namely, what base component they are a registration for (I can't remember if they provide this). The reason is that someone might want to have their own implementation of Button and we don't want to register myFancyButton if the end user has registered a substitute. We also don't want to double register it if they've handled it manually. I need to think about this more.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the warning:stale No recent activity within a reasonable amount of time label Apr 16, 2022
@janechu
Copy link
Collaborator

janechu commented May 29, 2024

This has been resolved in v2 of @microsoft/fast-element where defining the custom element can be achieved without a design system provider or custom registry.

Misunderstood the issue here, however I'm leaving this closed for now is the issue has gone stale without further activity. I think we can re-open it again if we have further potential solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-element Pertains to fast-element feature A new feature status:needs-author-input The PR needs input from the author warning:stale No recent activity within a reasonable amount of time
Projects
Status: Done
Development

No branches or pull requests

5 participants