-
Notifications
You must be signed in to change notification settings - Fork 36
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
chore(atomic): fix initialization issues when combining Lit interfaces with Stencil components #5070
base: master
Are you sure you want to change the base?
Conversation
Pull Request ReportPR Title✅ Title follows the conventional commit spec. Live demo linksBundle Size
|
function processLitDeclaration(declaration) { | ||
atomicAngularComponentFileContent += declarationToProxyCmp( | ||
declaration, | ||
`() => {customElements.get('${declaration.tagName}') || customElements.define('${declaration.tagName}', Lit${declaration.name});}` | ||
); | ||
litImports.add(declarationToLitImport(declaration)); | ||
litDeclarations.push(`${declaration.name}`); | ||
} | ||
|
||
function processNonLitDeclaration(declaration) { | ||
const defineCustomElementFn = `defineCustomElement${declaration.name}`; | ||
|
||
const regex = new RegExp( | ||
`@ProxyCmp\\(\\{([^}]*)\\}\\)\\s*\\n@Component\\(\\{([^}]*)\\}\\)\\s*\\nexport class\\s+${declaration.name}\\b`, | ||
'g' | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need a different defineCustomElement behaviour for Lit and Stencil components, we can't use the global defineCustomElements function from the loader.
For Lit components, we need to add a new component declaration that will use a custom defineCustomElement function.
For Stencil components, we want to add an import to the defineCustomElement function specific to that component provided by stencil and use it as the defineCustomElementsFn in the @ProxyCmp decorator of the component's declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fpbrault , did you explore removing the angular build steps from stencil.config.ts,
and use the same logic as lit for stencil, just with a different defineCustomElementsFn
?
Afterall, stencil components are also in the custom element manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did but some declarations include some extra things in their interface, and have a corresponding proxyOutputs call. It seemed easier to just add the defineCustomElementsFn to the existing declarations instead of handling that corner case. We can always revisit this later
if (!declaration.members) { | ||
continue; | ||
} | ||
|
||
declaration.members = declaration.members.filter( | ||
(member) => member?.privacy !== 'private' && member.kind === 'method' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed so that the component declarations in atomic-angular for Lit components include their methods (Stencil only includes methods marked with the @Method
decorator).
@@ -217,7 +217,7 @@ export class AtomicCommerceSearchBox | |||
} | |||
|
|||
public initialize() { | |||
this.id ??= randomID('atomic-commerce-search-box-'); | |||
this.id ||= randomID('atomic-commerce-search-box-'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for angular
@@ -232,5 +232,6 @@ export class AtomicProduct { | |||
|
|||
this.executedRenderingFunctionOnce = true; | |||
} | |||
this.host.classList.add('hydrated'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for angular
@@ -26,7 +26,7 @@ import { | |||
shadow: true, | |||
}) | |||
export class AtomicProductTemplate { | |||
private productTemplateCommon: ProductTemplateCommon; | |||
private productTemplateCommon!: ProductTemplateCommon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for angular
constructor() {} | ||
|
||
connectedCallback() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for angular, but it didn't make sense to have this code happen before the component is connected to DOM
@Listen('atomic/parentReady', {target: 'window'}) | ||
public handleParentReady(event: CustomEvent) { | ||
if (event.target === this.boundInterface) { | ||
markParentAsReady(this.host); | ||
} | ||
} | ||
|
||
/** | ||
* Represents the bound interface for the AtomicExternal component. | ||
*/ | ||
@Prop({mutable: true}) boundInterface?: AtomicInterface; | ||
|
||
connectedCallback() { | ||
if (isParentReady(this.#interface)) { | ||
markParentAsReady(this.host); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since atomic-external is an initializableElement but does not actually get initialized itself, we need to mark it as ready when its bound interface is ready.
@@ -47,7 +47,7 @@ export function watch< | |||
Prop extends PublicProperties<Component>, | |||
>(propName: Prop, options?: WatchOptions) { | |||
const resolvedOptions: Required<WatchOptions> = { | |||
waitUntilFirstUpdate: false, | |||
waitUntilFirstUpdate: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for angular
// import { AtomicCommerceInterface as AtomicCommerceInterface2 } from './components/commerce/atomic-commerce-interface/atomic-commerce-interface'; | ||
export namespace Components { | ||
/* | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
interface AtomicCommerceInterface extends AtomicCommerceInterface2 { | ||
} */ | ||
|
||
} | ||
|
||
declare global { | ||
/* | ||
interface HTMLAtomicCommerceInterfaceElement extends Components.AtomicCommerceInterface, HTMLElement { | ||
} | ||
let HTMLAtomicCommerceInterfaceElement: { | ||
prototype: HTMLAtomicCommerceInterfaceElement; | ||
new (): HTMLAtomicCommerceInterfaceElement; | ||
}; | ||
interface HTMLElementTagNameMap { | ||
'atomic-commerce-interface': HTMLAtomicCommerceInterfaceElement; | ||
} | ||
*/ | ||
} | ||
export {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't have the HTMLElementTagNameMap declarations in the components themselves, as it causes a type conflict when referencing them in atomic-angular. Additionally the HTMLAtomicElement interfaces in global and Atomic interfaces in the Components Namespace are needed for the angular sample to build when it references them.
If you have a better solution please mention it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mh... odd... let's split this issue in another ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be solved before we merge our lit components.
const parent = closest(element, initializableElements.join(', ')); | ||
|
||
console.log(parent); | ||
|
||
if (!parent) { | ||
reject(new MissingInterfaceParentError(element.nodeName.toLowerCase())); | ||
return; | ||
} | ||
|
||
if (isParentReady(parent)) { | ||
element.dispatchEvent(event); | ||
} else { | ||
queueEventForParent(parent, event as InitializeEvent, element); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of dispatching the event right away, components now check if their parent initializableElement is initialized first. If the parent is not ready, the event gets added to an event queue specific to that parent. Once the parent marks itself ready, it will dispatch all the queued event associated to itself.
https://coveord.atlassian.net/browse/KIT-4050 remove unintended changes https://coveord.atlassian.net/browse/KIT-4050 Add generated files remove log https://coveord.atlassian.net/browse/KIT-4050 fix test https://coveord.atlassian.net/browse/KIT-4050 fix issue with hash https://coveord.atlassian.net/browse/KIT-4050 simplify smoke tests https://coveord.atlassian.net/browse/KIT-4050 result templ https://coveord.atlassian.net/browse/KIT-4050 fix budget for angular sample https://coveord.atlassian.net/browse/KIT-4050 trying something https://coveord.atlassian.net/browse/KIT-4050 Add generated files add full fix https://coveord.atlassian.net/browse/KIT-4050 fix script https://coveord.atlassian.net/browse/KIT-4050 use custom getAssetPath https://coveord.atlassian.net/browse/KIT-4050 clean up https://coveord.atlassian.net/browse/KIT-4050 add generated files https://coveord.atlassian.net/browse/KIT-4050 fix unit tests https://coveord.atlassian.net/browse/KIT-4050 fix tests https://coveord.atlassian.net/browse/KIT-4050
This PR fixes initialization issues when combining LIt interfaces and Stencil components.
https://coveord.atlassian.net/browse/KIT-4050