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(components): enable prefix override #861

Merged
merged 43 commits into from
Dec 4, 2022
Merged

feat(components): enable prefix override #861

merged 43 commits into from
Dec 4, 2022

Conversation

yinonov
Copy link
Contributor

@yinonov yinonov commented Nov 24, 2022

custom prefix for custom elements

To enable integration of vivid components in micro-frontend architecture, where dev teams work on a same HTML document but with different - conflicting - versions of Vivid components. such scenarios require isolating custom-elements registration within the same document.

A similar scenario, where the need arises, is teams upgrading major - breaking changes - versions and prefer updating components gradually.

the feature yet exists in browsers as a proposal for scoped-elements is yet to be finalized.

A workaround for having same components (with different versions) at the same scope would be to customized their prefix and be registered under a pseudo prefix (e.g. <dashboard-badge>).

a recommendation by the FAST team is allow library-consuming teams to run the registration on their environment. meaning refactor our simple side-effects imports to a more controlled modules but this adds more work from consumers.

export const myDS = DesignSystem.getOrCreate();
myDS.withPrefix('foo').register(/* pre-register everything */)

or register a custom prefix for the components you are exporting

export function provideFASTDesignSystem(element?: HTMLElement): DesignSystem {
  return DesignSystem.getOrCreate(element).withPrefix('fast');
}

export const allComponents = {
  /* all of the component register functions */
  fastButton,
  fastAnchor,

  register(container?: Container, ...rest: any[]) {
    if (!container) {
      return
    }

    for (const key of this) {
      if (key === 'register') {
        return;
      }

      this[key]().register(container, ...rest);
    }
  }
}

Considering the above suggestions but keep the simplicity of consumption, this PR proposes keeping the simple side-effect imports but enable providing a parameter to be considered by the import.meta object.

With that, devs would be able to simple import with query parameter to define their desired prefix

<script type="module">
  import '/node_modules/@vonage/vivid/badge/index.js?prefix=dashboard';
</script>

<dashboard-badge text="some text"></dashboard-badge>

Build constraints

note that the query parameter doesn't delegate to nested imports within a module so any other imports within /node_modules/@vonage/vivid/badge/index.js can't access the prefix parameter.

that means rollup build must contain the meta code within each component output index.js. to better understand, check button's build (which fails to access the prefix parameter)

image

Compositions:

composite components (using context.tagFor to integrate other components) aren't aware of the custom context unless integrated components are imported with the same prefix query parameter.

in the following, icon is resolved as vwc-icon while its integrated, container has resolved as desired dashboard-badge.
This is due to the existing import within the badge module import '../icon'; which isn't resolved with that same meta context
image

to keep providing an ergonomic interface we should either log a warning for the prefix difference, or totally remove side effecting imports from within integrating components and document the necessity for authors to include the prerequisite imports. meaning an author wanting to use badge will need to also import icon.

import '/node_modules/@vonage/vivid/icon/index.js?prefix=dashboard';
import '/node_modules/@vonage/vivid/badge/index.js?prefix=dashboard';

👆Resolved to dynamically importing components modules

Testing

TBD

Suggestions -

  • Visual regression including the majority of components (custom prefixed) in a single document

@yinonov yinonov marked this pull request as ready for review November 26, 2022 08:52
@yinonov yinonov linked an issue Nov 28, 2022 that may be closed by this pull request
3 tasks
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #861 (6a440dd) into main (d61b119) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #861    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          123       154    +31     
  Lines         1562      2065   +503     
  Branches       108       131    +23     
==========================================
+ Hits          1562      2065   +503     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libs/components/src/lib/badge/badge.ts 100.00% <ø> (ø)
libs/components/src/lib/banner/banner.template.ts 100.00% <ø> (ø)
...rc/lib/breadcrumb-item/breadcrumb-item.template.ts 100.00% <ø> (ø)
libs/components/src/lib/button/button.template.ts 100.00% <ø> (ø)
libs/components/src/lib/fab/fab.template.ts 100.00% <ø> (ø)
libs/components/src/lib/header/header.template.ts 100.00% <ø> (ø)
libs/components/src/lib/layout/layout.ts 100.00% <ø> (ø)
.../src/lib/accordion-item/accordion-item.template.ts 100.00% <100.00%> (ø)
libs/components/src/lib/accordion-item/index.ts 100.00% <100.00%> (ø)
libs/components/src/lib/accordion/index.ts 100.00% <100.00%> (ø)
... and 104 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…nto prefix-override

# Conflicts:
#	libs/components/src/shared/utils/index.spec.ts
@yinonov yinonov added Focus: Documentation Improvements or additions to documentation Enhancement New feature or request Type: Feature Focus: Interface labels Dec 3, 2022
Comment on lines +10 to +13
beforeAll(async () => {
await customElements.whenDefined(COMPONENT_TAG);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not needed now as we have this code in the pattern itself, right?

Comment on lines +14 to +17
beforeAll(async () => {
await customElements.whenDefined(COMPONENT_TAG);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Comment on lines +12 to +15
beforeAll(async () => {
await customElements.whenDefined(COMPONENT_TAG);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question

@yinonov yinonov merged commit f023bd0 into main Dec 4, 2022
@yinonov yinonov deleted the prefix-override branch December 4, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Focus: Documentation Improvements or additions to documentation Focus: Interface Type: Feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Components] enable customized prefix
3 participants