Run & review this pull request in StackBlitz Codeflow.

@bctt Thanks for the PR! I'm just curious if there is a specific problem that this change solves? The reason I ask is because we only just recently made the opposite of this change, and it's only for our Lit-based components.

We use tryDefine() for the Lit-based components so it can gracefully handle potentially executing the function more than once. Ideally we do not attempt to define custom elements more than once of course, but this is just for backwards compatibility and consistency with the rest of legacy Forge.

Thanks again, I just want to better understand the motivation behind this one before the next steps.

Thanks for looking this over @DRiFTy17 ! The main drive behind this change was to have consistency across all of the defineComponent() calls in Forge.

After digging through forge-core, I now have a better understanding of how forge declares its components with the CustomComponentRegistry.
defineCustomComponent is already making a call to tryDefine, but it passes along the component's HTMLElementTagNameMap that is built into the component instead of taking in a different name as a parameter, like what is seen with forge-badge in the current version of badge/index.ts

I may be missing some knowledge if this change is a regression for those components though... was there somehow something that tryDefine was accomplishing that was more concise than what defineCustomComponent was doing with its logic that included tryDefine?

You're not missing anything, we're just in the middle of transition period right now between our legacy vanilla-based Web Component architecture and the new Lit-based components that everything will be eventually migrated to over time. We had a bug we needed to fix in the Lit-based components related to defineCustomElement() and we swapped to calling tryDefine() directly. We then came back recently in #889 to fix compatibility with the dependencies property in the Forge-based @customElement decorator, which subsequently also fixed the original usage of defineCustomElement()...

Sorry, I should have also provided a link to the change I mentioned above where this occurred: #889

That PR technically enables us to use defineCustomElement() again because it adds the missing static property to the Lit component classes (which is used to pass to tryDefine() as seen here).

A long term goal for Forge is to eliminate the need for these "definition functions" and static properties in favor of just side-effect imports like this for example:

import '@tylertech/forge/button';

Long story short, I think your change will be fine to revert back to defineCustomElement() for consistency but I just wanted to give you the full story of how it got in this state (sorry again for the confusion!). We'll need to do some testing on this by using in a separate app before releasing but that's about it!

🚀 PR was released in v3.9.0 🚀